fix(bridges): more streaming work (test infra, ffi fixes)#3768
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
📝 WalkthroughWalkthroughIntroduces a keyless LLM streaming replay harness: BAML HTTP ChangesLLM SSE Streaming Replay Harness
Sequence Diagram(s)sequenceDiagram
rect rgba(70, 130, 180, 0.5)
note over TestSuite,ReplayHandler: Test setup
TestSuite->>replay_serve_detached: call(recording_path)
replay_serve_detached-->>TestSuite: bound addr (host:port)
TestSuite->>TestSuite: set BAML_REPLAY_BASE_URL, BAML_REPLAY_API_KEY
end
rect rgba(60, 179, 113, 0.5)
note over TestSuite,HttpBodyStreaming: Streaming call execution
TestSuite->>stream_e2e_extract: invoke via BAML SDK
stream_e2e_extract->>ReplayHandler: POST /chat/completions (BAML_REPLAY_BASE_URL)
ReplayHandler->>SnapSSEFile: read .snap.sse fixture
ReplayHandler->>HttpBodyStreaming: new_streaming → write chunks → end
HttpBodyStreaming-->>stream_e2e_extract: SSE frames streamed via WireBody
stream_e2e_extract-->>TestSuite: stream partials + final value
end
rect rgba(210, 105, 30, 0.5)
note over TestSuite,ReplayHandler: Teardown
TestSuite->>ReplayHandler: POST /__replay__/shutdown
ReplayHandler->>replay_serve_detached: cancel token → reaper cancels serve task
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
⏭️ Performance benchmarks were skippedPerf benchmarks (CodSpeed) are opt-in on pull requests — they no longer run on every push. They always run automatically after merge to To run them on this PR, do any of the following, then push a commit (or re-run CI):
|
|
No description provided. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@baml_language/sdk_tests/crates/python_pydantic2/llm_functions/customizable/conftest.py`:
- Around line 95-125: The fixture has two lifecycle issues: startup failures
before yield skip the shutdown path, and environment variables are set without
restoration. Restructure the fixture to wrap the entire startup logic (the
deadline loop checking for error and addr_file, and the environment variable
assignments at lines 111-112) inside the try block before yield, so that the
finally block always executes even on startup timeouts or errors. Additionally,
before setting BAML_REPLAY_BASE_URL and BAML_REPLAY_API_KEY environment
variables, save their original values using os.environ.get(), and in the finally
block restore them using os.environ.pop() or by setting them back to their
original values to prevent environment pollution across tests.
In
`@baml_language/sdk_tests/crates/python_pydantic2/llm_functions/customizable/test_streaming_replay.py`:
- Around line 262-264: The stream-draining loops at lines 262-264 and 349-350
that call ReplayStreamExtractDoc_stream and check for StreamFinished have no
termination bounds, which means if stream completion regresses, these tests will
hang indefinitely instead of failing fast. Add iteration limits or timeouts to
both loop locations to ensure the tests fail quickly if the stream never
completes, for example by wrapping the loop with a maximum iteration counter or
using a timeout mechanism that breaks after a reasonable number of attempts.
In
`@baml_language/sdk_tests/crates/typescript_node/llm_functions/customizable/streaming_replay.test.ts`:
- Around line 95-110: The servePromise from ReplayServeUntilShutdown_async is
started but never monitored during the address file polling loop. If the serve
task fails early, that error remains undetected and the code continues polling
until the 10-second timeout expires, obscuring the actual failure. Modify the
polling loop to check if servePromise has rejected and immediately throw that
error rather than continuing to wait for the timeout. This can be done by using
Promise.race or by checking the promise's settlement status within each loop
iteration before checking the address file.
- Around line 118-124: The teardown code in the afterEach block awaits both the
shutdown fetch call to http://${h.addr}/__replay__/shutdown and the
h.servePromise without timeout limits, which can cause the test suite to hang
indefinitely if either operation gets stuck. Add timeout constraints to both
await statements to ensure they fail fast and allow the test suite to continue.
Use Promise.race or a similar timeout mechanism (such as Promise.all with
AbortSignal or setTimeout-based timeout wrappers) to enforce reasonable time
limits on both the fetch operation and the servePromise resolution.
In `@baml_language/sdk_tests/harness/llm_recordings/tests/recordings.rs`:
- Around line 259-269: The curl command being constructed in the external
capture call lacks a timeout configuration, which can cause the test to hang
indefinitely if the network or provider stalls. Add an explicit timeout argument
to the curl command by including a cmd.arg() call with curl's timeout option
(such as "--max-time" followed by an appropriate timeout duration in seconds)
before the cmd.output().expect("spawn curl") call executes the command.
- Around line 138-143: Locate the `capture_via_curl()` function and add timeout
flags to its curl invocation to prevent CI hangs on network stalls.
Specifically, add `--max-time` or `--connect-timeout` flags to the curl command
to enforce a reasonable timeout limit. Additionally, note that if the test
harness transitions from single-threaded to multi-threaded execution in the
future, the environment variable mutations at the env::set_var calls (for
BAML_REPLAY_BASE_URL and BAML_REPLAY_API_KEY) should be protected by a Mutex to
prevent race conditions across threads.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 0f2da415-8ba0-4a99-ac0f-855d98317649
⛔ Files ignored due to path filters (7)
baml_language/Cargo.lockis excluded by!**/*.lockbaml_language/crates/baml_tests/snapshots/baml_src/_root.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/baml_src/replay_server.snapis excluded by!**/*.snapbaml_language/sdk_tests/fixtures/llm_functions/recordings/replay_extract_doc.snapis excluded by!**/*.snapbaml_language/sdk_tests/fixtures/llm_functions/recordings/replay_extract_doc__request.snapis excluded by!**/*.snapbaml_language/sdk_tests/fixtures/llm_functions/recordings/replay_extract_string.snapis excluded by!**/*.snapbaml_language/sdk_tests/fixtures/llm_functions/recordings/replay_extract_string__request.snapis excluded by!**/*.snap
📒 Files selected for processing (17)
baml_language/Cargo.tomlbaml_language/crates/baml_tests/baml_src/ns_replay_server/replay_server.bamlbaml_language/sdk_tests/crates/python_pydantic2/llm_functions/customizable/conftest.pybaml_language/sdk_tests/crates/python_pydantic2/llm_functions/customizable/test_main.pybaml_language/sdk_tests/crates/python_pydantic2/llm_functions/customizable/test_streaming_class_e2e.pybaml_language/sdk_tests/crates/python_pydantic2/llm_functions/customizable/test_streaming_e2e.pybaml_language/sdk_tests/crates/python_pydantic2/llm_functions/customizable/test_streaming_replay.pybaml_language/sdk_tests/crates/typescript_node/llm_functions/customizable/streaming_replay.test.tsbaml_language/sdk_tests/fixtures/llm_functions/baml_src/ns_lorem/functions.bamlbaml_language/sdk_tests/fixtures/llm_functions/baml_src/ns_replay/replay_server.bamlbaml_language/sdk_tests/fixtures/llm_functions/recordings/.gitignorebaml_language/sdk_tests/fixtures/llm_functions/recordings/README.mdbaml_language/sdk_tests/fixtures/llm_functions/recordings/replay_extract_doc.snap.ssebaml_language/sdk_tests/fixtures/llm_functions/recordings/replay_extract_string.snap.ssebaml_language/sdk_tests/harness/llm_recordings/Cargo.tomlbaml_language/sdk_tests/harness/llm_recordings/src/lib.rsbaml_language/sdk_tests/harness/llm_recordings/tests/recordings.rs
Comment on lines
+262
to
+264
| stream = ReplayStreamExtractDoc_stream("ignored-by-replay-server") | ||
| while not isinstance(stream.next(), StreamFinished): | ||
| pass |
Contributor
There was a problem hiding this comment.
Add termination guards to detached/async stream-drain loops
Line 263 and Line 349 drain until StreamFinished without a bound. If stream completion regresses, these tests can hang the suite instead of failing fast.
Suggested fix
@@
- stream = ReplayStreamExtractDoc_stream("ignored-by-replay-server")
- while not isinstance(stream.next(), StreamFinished):
- pass
+ stream = ReplayStreamExtractDoc_stream("ignored-by-replay-server")
+ iterations = 0
+ while not isinstance(stream.next(), StreamFinished):
+ iterations += 1
+ assert iterations < 10_000, "detached stream.next() failed to terminate"
@@
- stream = await ReplayStreamExtractDoc_stream_async("ignored-by-replay-server")
- while not isinstance(await stream.next_async(), StreamFinished):
- pass
+ stream = await ReplayStreamExtractDoc_stream_async("ignored-by-replay-server")
+ iterations = 0
+ while not isinstance(await stream.next_async(), StreamFinished):
+ iterations += 1
+ assert iterations < 10_000, "async stream.next_async() failed to terminate"Also applies to: 349-350
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@baml_language/sdk_tests/crates/python_pydantic2/llm_functions/customizable/test_streaming_replay.py`
around lines 262 - 264, The stream-draining loops at lines 262-264 and 349-350
that call ReplayStreamExtractDoc_stream and check for StreamFinished have no
termination bounds, which means if stream completion regresses, these tests will
hang indefinitely instead of failing fast. Add iteration limits or timeouts to
both loop locations to ensure the tests fail quickly if the stream never
completes, for example by wrapping the loop with a maximum iteration counter or
using a timeout mechanism that breaks after a reasonable number of attempts.
Comment on lines
+95
to
+110
| const servePromise = replay.ReplayServeUntilShutdown_async(rec, addrFile); | ||
|
|
||
| let addr: string | undefined; | ||
| const deadline = performance.now() + 10_000; | ||
| while (performance.now() < deadline) { | ||
| if (fs.existsSync(addrFile)) { | ||
| const text = fs.readFileSync(addrFile, "utf8").trim(); | ||
| if (text) { | ||
| addr = text; | ||
| break; | ||
| } | ||
| } | ||
| await sleep(20); | ||
| } | ||
| if (!addr) throw new Error("replay server did not bind within 10s"); | ||
|
|
Contributor
There was a problem hiding this comment.
Surface serve-task failures during bind polling (don’t wait for the 10s timeout).
Line 95 starts ReplayServeUntilShutdown_async, but Lines 97-109 only poll the addr-file. If the serve task rejects early, this path still waits until timeout and obscures the real failure.
Proposed fix
const servePromise = replay.ReplayServeUntilShutdown_async(rec, addrFile);
+ let serveError: unknown;
+ servePromise.catch((e) => {
+ serveError = e;
+ });
let addr: string | undefined;
const deadline = performance.now() + 10_000;
while (performance.now() < deadline) {
+ if (serveError) {
+ throw new Error(`replay server failed before bind: ${String(serveError)}`);
+ }
if (fs.existsSync(addrFile)) {
const text = fs.readFileSync(addrFile, "utf8").trim();
if (text) {
addr = text;
break;
}
}
await sleep(20);
}📝 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
| const servePromise = replay.ReplayServeUntilShutdown_async(rec, addrFile); | |
| let addr: string | undefined; | |
| const deadline = performance.now() + 10_000; | |
| while (performance.now() < deadline) { | |
| if (fs.existsSync(addrFile)) { | |
| const text = fs.readFileSync(addrFile, "utf8").trim(); | |
| if (text) { | |
| addr = text; | |
| break; | |
| } | |
| } | |
| await sleep(20); | |
| } | |
| if (!addr) throw new Error("replay server did not bind within 10s"); | |
| const servePromise = replay.ReplayServeUntilShutdown_async(rec, addrFile); | |
| let serveError: unknown; | |
| servePromise.catch((e) => { | |
| serveError = e; | |
| }); | |
| let addr: string | undefined; | |
| const deadline = performance.now() + 10_000; | |
| while (performance.now() < deadline) { | |
| if (serveError) { | |
| throw new Error(`replay server failed before bind: ${String(serveError)}`); | |
| } | |
| if (fs.existsSync(addrFile)) { | |
| const text = fs.readFileSync(addrFile, "utf8").trim(); | |
| if (text) { | |
| addr = text; | |
| break; | |
| } | |
| } | |
| await sleep(20); | |
| } | |
| if (!addr) throw new Error("replay server did not bind within 10s"); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@baml_language/sdk_tests/crates/typescript_node/llm_functions/customizable/streaming_replay.test.ts`
around lines 95 - 110, The servePromise from ReplayServeUntilShutdown_async is
started but never monitored during the address file polling loop. If the serve
task fails early, that error remains undetected and the code continues polling
until the 10-second timeout expires, obscuring the actual failure. Modify the
polling loop to check if servePromise has rejected and immediately throw that
error rather than continuing to wait for the timeout. This can be done by using
Promise.race or by checking the promise's settlement status within each loop
iteration before checking the address file.
Comment on lines
+118
to
+124
| await fetch(`http://${h.addr}/__replay__/shutdown`, { method: "POST", body: "" }); | ||
| } catch { | ||
| /* tolerate the response/cancel race */ | ||
| } | ||
| try { | ||
| await h.servePromise; | ||
| } catch { |
Contributor
There was a problem hiding this comment.
Bound teardown waits with timeouts to prevent suite hangs.
Lines 118-124 await both shutdown fetch and servePromise without timeout limits. A stuck local HTTP call or unresolved serve task can hang afterEach and stall the test run.
Proposed fix
async function stopReplayServer(h: ReplayHandle): Promise<void> {
try {
- await fetch(`http://${h.addr}/__replay__/shutdown`, { method: "POST", body: "" });
+ await fetch(`http://${h.addr}/__replay__/shutdown`, {
+ method: "POST",
+ body: "",
+ signal: AbortSignal.timeout(5_000),
+ });
} catch {
/* tolerate the response/cancel race */
}
try {
- await h.servePromise;
+ await Promise.race([
+ h.servePromise,
+ sleep(10_000).then(() => {
+ throw new Error("timed out waiting for replay serve task to finish");
+ }),
+ ]);
} catch {
/* serve task cancelled */
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@baml_language/sdk_tests/crates/typescript_node/llm_functions/customizable/streaming_replay.test.ts`
around lines 118 - 124, The teardown code in the afterEach block awaits both the
shutdown fetch call to http://${h.addr}/__replay__/shutdown and the
h.servePromise without timeout limits, which can cause the test suite to hang
indefinitely if either operation gets stuck. Add timeout constraints to both
await statements to ensure they fail fast and allow the test suite to continue.
Use Promise.race or a similar timeout mechanism (such as Promise.all with
AbortSignal or setTimeout-based timeout wrappers) to enforce reasonable time
limits on both the fetch operation and the servePromise resolution.
Comment on lines
+138
to
+143
| // `env.VAR` in the client options desugars to `baml.env.get_or_panic`, | ||
| // read when the client is instantiated inside the builder — so set them | ||
| // before running. (Process-global; nextest isolates each test in its own | ||
| // process. Under plain `cargo test` these would race across threads.) | ||
| env::set_var("BAML_REPLAY_BASE_URL", OPENAI_BASE_URL); | ||
| env::set_var("BAML_REPLAY_API_KEY", api_key); |
Contributor
There was a problem hiding this comment.
Add timeout to the curl invocation to prevent CI hangs.
The capture_via_curl() function at lines 335 makes an external network call without timeout flags, which can block CI indefinitely on network stalls or provider latency. Add --max-time or --connect-timeout to prevent hanging.
Regarding lines 142–143: The current test configuration uses #[tokio::test] with default single-threaded runtime, so env var mutations do not create practical concurrency issues. If multi-threaded execution is planned, serialization via a Mutex would be necessary.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@baml_language/sdk_tests/harness/llm_recordings/tests/recordings.rs` around
lines 138 - 143, Locate the `capture_via_curl()` function and add timeout flags
to its curl invocation to prevent CI hangs on network stalls. Specifically, add
`--max-time` or `--connect-timeout` flags to the curl command to enforce a
reasonable timeout limit. Additionally, note that if the test harness
transitions from single-threaded to multi-threaded execution in the future, the
environment variable mutations at the env::set_var calls (for
BAML_REPLAY_BASE_URL and BAML_REPLAY_API_KEY) should be protected by a Mutex to
prevent race conditions across threads.
Comment on lines
+259
to
+269
| cmd.arg("--silent") | ||
| .arg("--no-buffer") | ||
| .arg("--fail-with-body") | ||
| .arg("-X") | ||
| .arg("POST") | ||
| .arg(&req.url); | ||
| for (k, v) in &req.headers { | ||
| cmd.arg("-H").arg(format!("{k}: {v}")); | ||
| } | ||
| cmd.arg("--data").arg(&req.body); | ||
| let out = cmd.output().expect("spawn curl"); |
Contributor
There was a problem hiding this comment.
Add an explicit curl timeout to prevent indefinite hangs in recording mode.
The external capture call has no timeout, so a stalled network/provider can block this test job indefinitely.
Suggested fix
fn capture_via_curl(req: &RequestParts) -> Vec<u8> {
let mut cmd = Command::new("curl");
cmd.arg("--silent")
.arg("--no-buffer")
+ .arg("--connect-timeout").arg("10")
+ .arg("--max-time").arg("120")
.arg("--fail-with-body")
.arg("-X")
.arg("POST")
.arg(&req.url);📝 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
| cmd.arg("--silent") | |
| .arg("--no-buffer") | |
| .arg("--fail-with-body") | |
| .arg("-X") | |
| .arg("POST") | |
| .arg(&req.url); | |
| for (k, v) in &req.headers { | |
| cmd.arg("-H").arg(format!("{k}: {v}")); | |
| } | |
| cmd.arg("--data").arg(&req.body); | |
| let out = cmd.output().expect("spawn curl"); | |
| cmd.arg("--silent") | |
| .arg("--no-buffer") | |
| .arg("--connect-timeout").arg("10") | |
| .arg("--max-time").arg("120") | |
| .arg("--fail-with-body") | |
| .arg("-X") | |
| .arg("POST") | |
| .arg(&req.url); | |
| for (k, v) in &req.headers { | |
| cmd.arg("-H").arg(format!("{k}: {v}")); | |
| } | |
| cmd.arg("--data").arg(&req.body); | |
| let out = cmd.output().expect("spawn curl"); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@baml_language/sdk_tests/harness/llm_recordings/tests/recordings.rs` around
lines 259 - 269, The curl command being constructed in the external capture call
lacks a timeout configuration, which can cause the test to hang indefinitely if
the network or provider stalls. Add an explicit timeout argument to the curl
command by including a cmd.arg() call with curl's timeout option (such as
"--max-time" followed by an appropriate timeout duration in seconds) before the
cmd.output().expect("spawn curl") call executes the command.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
baml_language/sdk_tests/harness/llm_recordings/tests/recordings.rs (1)
223-236:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd explicit curl timeouts to prevent hanging recording jobs.
capture_via_curlcalls an external provider without connection/overall timeout bounds, so network stalls can block this test indefinitely.Suggested fix
fn capture_via_curl(req: &RequestParts) -> Vec<u8> { let mut cmd = Command::new("curl"); cmd.arg("--silent") .arg("--no-buffer") + .arg("--connect-timeout") + .arg("10") + .arg("--max-time") + .arg("120") .arg("--fail-with-body") .arg("-X") .arg(&req.method) .arg(&req.url);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@baml_language/sdk_tests/harness/llm_recordings/tests/recordings.rs` around lines 223 - 236, The `capture_via_curl` function executes curl without timeout constraints, which can cause the test to hang indefinitely during network stalls. Add explicit timeout arguments to the curl command before executing it (before the cmd.output() call). Use curl's --connect-timeout flag for connection establishment timeout and --max-time flag for overall operation timeout to ensure the command terminates within a reasonable timeframe even if the external provider is unresponsive.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@baml_language/sdk_tests/harness/llm_recordings/tests/recordings.rs`:
- Around line 223-236: The `capture_via_curl` function executes curl without
timeout constraints, which can cause the test to hang indefinitely during
network stalls. Add explicit timeout arguments to the curl command before
executing it (before the cmd.output() call). Use curl's --connect-timeout flag
for connection establishment timeout and --max-time flag for overall operation
timeout to ensure the command terminates within a reasonable timeframe even if
the external provider is unresponsive.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d33c0614-2d81-4f11-af07-99439f23d755
⛔ Files ignored due to path filters (1)
baml_language/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
baml_language/sdk_tests/crates/python_pydantic2/llm_functions/customizable/test_streaming_replay.pybaml_language/sdk_tests/crates/typescript_node/llm_functions/customizable/streaming_replay.test.tsbaml_language/sdk_tests/fixtures/llm_functions/recordings/README.mdbaml_language/sdk_tests/fixtures/llm_functions/recordings/replay_extract_doc.snap.ssebaml_language/sdk_tests/fixtures/llm_functions/recordings/replay_extract_string.snap.ssebaml_language/sdk_tests/harness/llm_recordings/Cargo.tomlbaml_language/sdk_tests/harness/llm_recordings/data/replay_server.bamlbaml_language/sdk_tests/harness/llm_recordings/tests/recordings.rsbaml_language/sdk_tests/harness/llm_recordings/tests/transport_pin.rs
💤 Files with no reviewable changes (1)
- baml_language/sdk_tests/harness/llm_recordings/Cargo.toml
✅ Files skipped from review due to trivial changes (2)
- baml_language/sdk_tests/fixtures/llm_functions/recordings/README.md
- baml_language/sdk_tests/fixtures/llm_functions/recordings/replay_extract_string.snap.sse
🚧 Files skipped from review as they are similar to previous changes (3)
- baml_language/sdk_tests/fixtures/llm_functions/recordings/replay_extract_doc.snap.sse
- baml_language/sdk_tests/crates/python_pydantic2/llm_functions/customizable/test_streaming_replay.py
- baml_language/sdk_tests/crates/typescript_node/llm_functions/customizable/streaming_replay.test.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
baml_language/sdk_tests/crates/python_pydantic2/llm_functions/customizable/conftest.py (1)
131-156:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMove startup polling inside the try block to guarantee cleanup.
The polling loop (lines 131-146) executes before the
tryblock at line 156. If startup times out orerroris raised (lines 134-135, 142-146), thefinallyblock never runs, leaving the thread un-joined and env vars unrestored.🔧 Proposed fix
t = threading.Thread(target=_serve, daemon=True) t.start() - addr = None - deadline = time.time() + 10.0 - while time.time() < deadline: - if error: - raise RuntimeError(f"replay server thread failed: {error[0]!r}") - if addr_file.exists(): - text = addr_file.read_text().strip() - if text: - addr = text - break - time.sleep(0.02) - if addr is None: - raise TimeoutError( - f"replay server did not bind within 10s " - f"(thread alive={t.is_alive()}, error={error!r})" - ) - # Save + restore the client env so a torn-down server's (dead) address # never leaks into a later test that forgot to set it. saved = { k: os.environ.get(k) for k in ("BAML_REPLAY_BASE_URL", "BAML_REPLAY_API_KEY") } - os.environ["BAML_REPLAY_BASE_URL"] = f"http://{addr}" - os.environ["BAML_REPLAY_API_KEY"] = "replay-test-key" try: + addr = None + deadline = time.time() + 10.0 + while time.time() < deadline: + if error: + raise RuntimeError(f"replay server thread failed: {error[0]!r}") + if addr_file.exists(): + text = addr_file.read_text().strip() + if text: + addr = text + break + time.sleep(0.02) + if addr is None: + raise TimeoutError( + f"replay server did not bind within 10s " + f"(thread alive={t.is_alive()}, error={error!r})" + ) + + os.environ["BAML_REPLAY_BASE_URL"] = f"http://{addr}" + os.environ["BAML_REPLAY_API_KEY"] = "replay-test-key" yield addr finally:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@baml_language/sdk_tests/crates/python_pydantic2/llm_functions/customizable/conftest.py` around lines 131 - 156, The startup polling loop that waits for the replay server to bind (starting with the while loop checking time.time() < deadline and ending with the TimeoutError raise) is currently positioned before the try block. This means if a RuntimeError is raised due to the thread failing, or if a TimeoutError is raised when the server doesn't bind within 10 seconds, the finally block will never execute, leaving the thread un-joined and environment variables unrestored. Move the entire polling section (the while loop and the subsequent TimeoutError check) inside the try block so that any startup failures will guarantee the cleanup code in the finally block runs.
♻️ Duplicate comments (3)
baml_language/sdk_tests/harness/llm_recordings/tests/recordings.rs (1)
208-228:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd timeout flags to prevent curl from hanging indefinitely.
The curl command lacks
--connect-timeoutand--max-timeflags. Network stalls or provider latency can block test execution indefinitely.⏱️ Proposed fix
fn capture_via_curl(req: &RequestParts) -> Vec<u8> { let mut cmd = Command::new("curl"); cmd.arg("--silent") .arg("--no-buffer") + .arg("--connect-timeout").arg("10") + .arg("--max-time").arg("120") .arg("--fail-with-body") .arg("-X")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@baml_language/sdk_tests/harness/llm_recordings/tests/recordings.rs` around lines 208 - 228, The capture_via_curl function's curl command lacks timeout protection, which can cause tests to hang indefinitely during network stalls or provider latency. Add --connect-timeout and --max-time flags to the curl command builder in the capture_via_curl function to set both a connection timeout and a maximum overall request time, placing these arguments with the other cmd.arg() calls before executing cmd.output().baml_language/sdk_tests/crates/python_pydantic2/llm_functions/customizable/test_streaming_replay.py (2)
199-200:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd iteration bound to prevent infinite loop in asyncio mode test.
The
whileloop drainingstream.next_async()has no iteration counter. If stream completion regresses, the test will hang indefinitely instead of failing fast.🔒 Proposed fix
stream = await StreamE2EExtractDoc_stream_async("ignored-by-replay-server") - while not isinstance(await stream.next_async(), StreamFinished): - pass + iterations = 0 + while not isinstance(await stream.next_async(), StreamFinished): + iterations += 1 + assert iterations < 10_000, "async stream.next_async() failed to terminate"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@baml_language/sdk_tests/crates/python_pydantic2/llm_functions/customizable/test_streaming_replay.py` around lines 199 - 200, The while loop draining stream.next_async() in the test lacks an iteration bound, which will cause the test to hang indefinitely if stream completion regresses. Add a counter variable to track the number of iterations and establish a reasonable maximum iteration limit. If the counter exceeds this limit before encountering StreamFinished, raise an exception or assertion failure to fail the test fast instead of hanging. This ensures the test properly detects regressions in stream completion behavior.
119-121:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd iteration bound to prevent infinite loop in detached mode test.
The
whileloop drainingstream.next()has no iteration counter. If stream completion regresses, the test will hang indefinitely instead of failing fast.🔒 Proposed fix
stream = StreamE2EExtractDoc_stream("ignored-by-replay-server") - while not isinstance(stream.next(), StreamFinished): - pass + iterations = 0 + while not isinstance(stream.next(), StreamFinished): + iterations += 1 + assert iterations < 10_000, "detached stream.next() failed to terminate"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@baml_language/sdk_tests/crates/python_pydantic2/llm_functions/customizable/test_streaming_replay.py` around lines 119 - 121, The while loop in the test_streaming_replay function that drains stream.next() calls has no iteration limit, which will cause the test to hang indefinitely if the stream fails to complete properly. Add an iteration counter before the while loop and increment it on each iteration, then assert or raise an exception if the counter exceeds a reasonable maximum value (e.g., 1000) before StreamFinished is received. This ensures the test fails fast with a clear error message rather than hanging when stream completion regresses.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@baml_language/sdk_tests/crates/python_pydantic2/llm_functions/customizable/test_streaming_class_e2e.py`:
- Around line 102-103: Add iteration bounds to prevent infinite hangs in the
three while loops that drain streams until StreamFinished is received. For each
while loop that calls stream.next() without bounds (one at lines 102-103, one at
lines 115-116, and one at lines 137-138), introduce a counter variable
initialized to zero and increment it on each iteration. Set a reasonable maximum
limit (such as 1000 iterations) and assert or raise an exception if the counter
exceeds this limit before StreamFinished is received. This ensures tests fail
fast with a clear error message rather than hanging indefinitely if stream
completion regresses.
- Around line 81-82: The while loop that drains the stream by repeatedly calling
stream.next_async() lacks an iteration counter, which means the test will hang
indefinitely if stream completion regresses instead of failing quickly. Add an
iteration counter to the loop that tracks how many times stream.next_async() is
called and raise an exception or assertion failure if a reasonable maximum
iteration limit is exceeded without the stream returning a StreamFinished
instance.
In
`@baml_language/sdk_tests/crates/python_pydantic2/llm_functions/customizable/test_streaming_e2e.py`:
- Around line 181-182: The test file has two `while` loops (at lines 181-182 and
lines 228-229) that drain streams by calling stream.next() until receiving a
StreamFinished event, but they lack iteration limits. If stream completion
regresses, these loops will hang indefinitely instead of failing fast. Add
iteration counters to both loops with a reasonable maximum iteration limit
(e.g., 1000 iterations), and raise an assertion error or fail the test if the
limit is exceeded before StreamFinished is received. This ensures the tests
timeout quickly during failures rather than hanging.
---
Outside diff comments:
In
`@baml_language/sdk_tests/crates/python_pydantic2/llm_functions/customizable/conftest.py`:
- Around line 131-156: The startup polling loop that waits for the replay server
to bind (starting with the while loop checking time.time() < deadline and ending
with the TimeoutError raise) is currently positioned before the try block. This
means if a RuntimeError is raised due to the thread failing, or if a
TimeoutError is raised when the server doesn't bind within 10 seconds, the
finally block will never execute, leaving the thread un-joined and environment
variables unrestored. Move the entire polling section (the while loop and the
subsequent TimeoutError check) inside the try block so that any startup failures
will guarantee the cleanup code in the finally block runs.
---
Duplicate comments:
In
`@baml_language/sdk_tests/crates/python_pydantic2/llm_functions/customizable/test_streaming_replay.py`:
- Around line 199-200: The while loop draining stream.next_async() in the test
lacks an iteration bound, which will cause the test to hang indefinitely if
stream completion regresses. Add a counter variable to track the number of
iterations and establish a reasonable maximum iteration limit. If the counter
exceeds this limit before encountering StreamFinished, raise an exception or
assertion failure to fail the test fast instead of hanging. This ensures the
test properly detects regressions in stream completion behavior.
- Around line 119-121: The while loop in the test_streaming_replay function that
drains stream.next() calls has no iteration limit, which will cause the test to
hang indefinitely if the stream fails to complete properly. Add an iteration
counter before the while loop and increment it on each iteration, then assert or
raise an exception if the counter exceeds a reasonable maximum value (e.g.,
1000) before StreamFinished is received. This ensures the test fails fast with a
clear error message rather than hanging when stream completion regresses.
In `@baml_language/sdk_tests/harness/llm_recordings/tests/recordings.rs`:
- Around line 208-228: The capture_via_curl function's curl command lacks
timeout protection, which can cause tests to hang indefinitely during network
stalls or provider latency. Add --connect-timeout and --max-time flags to the
curl command builder in the capture_via_curl function to set both a connection
timeout and a maximum overall request time, placing these arguments with the
other cmd.arg() calls before executing cmd.output().
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: bdbe12af-5fed-40ab-b9b8-3c8a1a106f69
📒 Files selected for processing (10)
baml_language/sdk_tests/crates/python_pydantic2/llm_functions/customizable/conftest.pybaml_language/sdk_tests/crates/python_pydantic2/llm_functions/customizable/test_main.pybaml_language/sdk_tests/crates/python_pydantic2/llm_functions/customizable/test_streaming_class_e2e.pybaml_language/sdk_tests/crates/python_pydantic2/llm_functions/customizable/test_streaming_e2e.pybaml_language/sdk_tests/crates/python_pydantic2/llm_functions/customizable/test_streaming_replay.pybaml_language/sdk_tests/crates/typescript_node/llm_functions/customizable/streaming_e2e.test.tsbaml_language/sdk_tests/crates/typescript_node/llm_functions/customizable/streaming_replay.test.tsbaml_language/sdk_tests/fixtures/llm_functions/baml_src/ns_lorem/functions.bamlbaml_language/sdk_tests/fixtures/llm_functions/recordings/README.mdbaml_language/sdk_tests/harness/llm_recordings/tests/recordings.rs
✅ Files skipped from review due to trivial changes (1)
- baml_language/sdk_tests/fixtures/llm_functions/recordings/README.md
🚧 Files skipped from review as they are similar to previous changes (1)
- baml_language/sdk_tests/crates/typescript_node/llm_functions/customizable/streaming_replay.test.ts
…riven streams) Host-driven $stream handles failed to round-trip on the Node bridge because the inbound ADT_TAGGED_HEAP_HANDLE tag had no decode arm. Decode it via the typemap so Stream handles cross the bridge. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This comment has been minimized.
This comment has been minimized.
Add incremental server responses to baml.http.Server: Response.new_streaming/.write/.end send the body in chunks (chunked transfer-encoding, channel-backed with backpressure), implemented across baml_std, sys_native, sys_ops, bridge_wasm, and the playground proxy. Regenerate baml_std snapshots for the new stdlib functions. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…-call return boundary A generic instance method invoked from the host (e.g. Stream.next, by name via call_function_bound_args or by handle via call_callable) kept its declared return-type class type vars unsubstituted, so a concrete value (a stream partial) matched no member of a union like TStream | StreamFinished and the FFI return conversion panicked. Recover TypeVar->concrete bindings by zipping the declared self param type against the receiver concrete self (the inbound TaggedHeapHandle Class args, or the seeded class_type_args) and substitute the declared return type before conversion. New conversion.rs helpers: collect_type_var_bindings, substitute_type_vars, tagged_handle_runtime_ty. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…tures) Recorder crate that captures real provider SSE via curl into insta binary snapshots, plus the checked-in recordings and an engine-level SSE transport pin. Registers the crate in the workspace. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
sxlijin
changed the title
Python (conftest/replay/e2e/class-e2e) and TypeScript (replay/e2e) streaming tests driving the BAML-implemented keyless replay server, plus the StreamE2E fixtures and the env-driven replay server fixture. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
baml_language/crates/bex_engine/src/lib.rs (1)
1867-1902:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winSubstitute bound class type vars in
throws_typetoo.
run_entry_pointalso usesthrows_typeto type escaping throws. Leaving it generic preserves the same unbound-TypeVarfailure for methods whosethrowsclause mentions the receiver’s class type params.Apply the same bindings to `throws_type`
- let throws_type = self.function_throws_type(function_name); + let mut throws_type = self.function_throws_type(function_name); @@ crate::conversion::collect_type_var_bindings( self_declared, self_actual, &mut bindings, ); return_type = crate::conversion::substitute_type_vars(&return_type, &bindings); + if let Some(throws_type) = throws_type.as_mut() { + *throws_type = + crate::conversion::substitute_type_vars(throws_type, &bindings); + } } } @@ - let (mut return_type, throws_type, arity, param_types) = + let (mut return_type, mut throws_type, arity, param_types) = match thread.vm.get_object(func_ptr) { @@ for (declared, concrete) in declared_args.iter().zip(seed_type_args.iter()) { crate::conversion::collect_type_var_bindings(declared, concrete, &mut bindings); } return_type = crate::conversion::substitute_type_vars(&return_type, &bindings); + if let Some(throws_type) = throws_type.as_mut() { + *throws_type = + crate::conversion::substitute_type_vars(throws_type, &bindings); + } } }Also applies to: 2154-2192
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@baml_language/crates/bex_engine/src/lib.rs` around lines 1867 - 1902, The code collects type variable bindings from the receiver and applies them to the `return_type` through the `crate::conversion::substitute_type_vars` function, but fails to apply the same substitution to `throws_type`. This causes throws clauses that reference the receiver's class type parameters to remain unbound. Apply the same `crate::conversion::substitute_type_vars` substitution to `throws_type` using the collected `bindings` HashMap in the same conditional block where `return_type` is being substituted, ensuring both return and throws types are properly specialized.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@baml_language/crates/baml_builtins2/baml_std/baml/ns_http/http.baml`:
- Around line 113-116: The _timeout_nanos function collapses two distinct
timeout states into the same value: the null case returns 0n for "no timeout"
and a zero Duration also returns 0n when to_nanoseconds() is called, making
fetch or send with a zero timeout run unbounded instead of timing out
immediately. To fix this, change the null pattern match case in _timeout_nanos
to return a negative value (such as -1n) instead of 0n to represent "no
deadline", while allowing zero Duration objects to return 0n for an actual
zero-duration timeout. Then update the native timeout decoding logic that
interprets these values to treat negative values as "no deadline" and preserve 0
as a real zero-duration timeout.
In `@baml_language/crates/bex_engine/src/conversion.rs`:
- Around line 762-783: The Class branch in collect_type_var_bindings silently
processes mismatched classes without verifying they are the same class and have
the same arity, and the Union branch silently truncates when member counts
differ. Add guard checks in both branches to validate class identity and ensure
matching arity in the Class case (comparing the class identifiers and the
lengths of the type argument vectors), and ensure matching union member counts
in the Union case before proceeding with zip iteration. Add a unit test for
collect_type_var_bindings that covers both mismatched class scenarios and
mismatched union scenarios to verify the guards work correctly, then run cargo
test --lib to confirm all tests pass.
In
`@baml_language/sdk_tests/crates/python_pydantic2/llm_functions/customizable/replay_harness.py`:
- Line 76: After the thread.join(timeout=10) call in the teardown, add a
verification check to ensure the replay server thread has actually terminated.
If the thread is still alive after the timeout expires, raise an exception to
fail fast instead of silently continuing, which would allow a leaked daemon
server to potentially cause flaky tests. Use the thread's is_alive() method to
check termination status and handle the case where the shutdown stalls.
- Around line 67-80: The teardown code in the finally block unconditionally
deletes BAML_REPLAY_BASE_URL and BAML_REPLAY_API_KEY environment variables,
which corrupts test state if these variables were already set before entering
the harness. Save the original values of these environment variables before
setting them at the start of the context manager, and then restore them to their
original values in the finally block instead of unconditionally popping them. If
a variable had a prior value, restore it; if it didn't exist before, remove it
from the environment.
In
`@baml_language/sdk_tests/crates/typescript_node/llm_functions/customizable/replay_harness.ts`:
- Around line 43-55: The `withReplayServer` function unconditionally deletes the
replay environment variables without preserving their original values, which can
leak state across tests if those variables were preconfigured. Before setting
`process.env.BAML_REPLAY_BASE_URL` and `process.env.BAML_REPLAY_API_KEY`, store
their original values (if they exist). In the finally block, instead of using
delete operations on these variables, restore them to their original values (or
delete them only if they weren't previously set).
---
Outside diff comments:
In `@baml_language/crates/bex_engine/src/lib.rs`:
- Around line 1867-1902: The code collects type variable bindings from the
receiver and applies them to the `return_type` through the
`crate::conversion::substitute_type_vars` function, but fails to apply the same
substitution to `throws_type`. This causes throws clauses that reference the
receiver's class type parameters to remain unbound. Apply the same
`crate::conversion::substitute_type_vars` substitution to `throws_type` using
the collected `bindings` HashMap in the same conditional block where
`return_type` is being substituted, ensuring both return and throws types are
properly specialized.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ab6bbc4d-1d59-40a0-8de4-54d557a9e7fb
⛔ Files ignored due to path filters (14)
baml_language/Cargo.lockis excluded by!**/*.lockbaml_language/crates/baml_cli/src/snapshots/baml_cli__describe_command_tests__render_builtin_package_listing.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/__baml_std__/baml_tests__compiles____baml_std____03_hir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/__baml_std__/baml_tests__compiles____baml_std____04_5_mir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/__baml_std__/baml_tests__compiles____baml_std____06_codegen.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/src/compiler2_tir/snapshots/baml_tests__compiler2_tir__phase5__snapshot_baml_package_items.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/tests/bytecode_format/snapshots/bytecode_format__bytecode_display_expanded.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/tests/bytecode_format/snapshots/bytecode_format__bytecode_display_expanded_unoptimized.snapis excluded by!**/*.snapbaml_language/sdk_tests/fixtures/llm_functions/recordings/replay_extract_doc.snapis excluded by!**/*.snapbaml_language/sdk_tests/fixtures/llm_functions/recordings/replay_extract_string.snapis excluded by!**/*.snapbaml_language/sdks/nodejs/bridge_nodejs/dist/proto.d.ts.mapis excluded by!**/dist/**,!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/dist/proto.jsis excluded by!**/dist/**baml_language/sdks/nodejs/bridge_nodejs/dist/proto.js.mapis excluded by!**/dist/**,!**/*.mapbaml_language/sdks/python/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (28)
baml_language/Cargo.tomlbaml_language/crates/baml_builtins2/baml_std/baml/ns_http/http.bamlbaml_language/crates/baml_lsp_server/src/playground_http.rsbaml_language/crates/bex_engine/src/conversion.rsbaml_language/crates/bex_engine/src/lib.rsbaml_language/crates/bridge_wasm/src/wasm_http.rsbaml_language/crates/sys_native/src/http_server.rsbaml_language/crates/sys_native/src/io_impls.rsbaml_language/crates/sys_ops/src/lib.rsbaml_language/sdk_tests/crates/python_pydantic2/llm_functions/customizable/replay_harness.pybaml_language/sdk_tests/crates/python_pydantic2/llm_functions/customizable/test_main.pybaml_language/sdk_tests/crates/python_pydantic2/llm_functions/customizable/test_streaming_class_e2e.pybaml_language/sdk_tests/crates/python_pydantic2/llm_functions/customizable/test_streaming_e2e.pybaml_language/sdk_tests/crates/typescript_node/llm_functions/customizable/replay_harness.tsbaml_language/sdk_tests/crates/typescript_node/llm_functions/customizable/streaming_e2e.test.tsbaml_language/sdk_tests/fixtures/llm_functions/baml_src/ns_lorem/functions.bamlbaml_language/sdk_tests/fixtures/llm_functions/baml_src/ns_lorem/types.bamlbaml_language/sdk_tests/fixtures/llm_functions/baml_src/ns_replay/replay_server.bamlbaml_language/sdk_tests/fixtures/llm_functions/recordings/.gitignorebaml_language/sdk_tests/fixtures/llm_functions/recordings/README.mdbaml_language/sdk_tests/fixtures/llm_functions/recordings/replay_extract_doc.snap.ssebaml_language/sdk_tests/fixtures/llm_functions/recordings/replay_extract_string.snap.ssebaml_language/sdk_tests/harness/llm_recordings/Cargo.tomlbaml_language/sdk_tests/harness/llm_recordings/data/replay_server.bamlbaml_language/sdk_tests/harness/llm_recordings/src/lib.rsbaml_language/sdk_tests/harness/llm_recordings/tests/recordings.rsbaml_language/sdk_tests/harness/llm_recordings/tests/transport_pin.rsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/proto.ts
💤 Files with no reviewable changes (1)
- baml_language/sdk_tests/crates/python_pydantic2/llm_functions/customizable/test_streaming_class_e2e.py
✅ Files skipped from review due to trivial changes (6)
- baml_language/sdk_tests/fixtures/llm_functions/recordings/.gitignore
- baml_language/sdk_tests/fixtures/llm_functions/baml_src/ns_lorem/types.baml
- baml_language/sdk_tests/harness/llm_recordings/src/lib.rs
- baml_language/sdk_tests/fixtures/llm_functions/recordings/README.md
- baml_language/sdk_tests/fixtures/llm_functions/recordings/replay_extract_doc.snap.sse
- baml_language/sdk_tests/fixtures/llm_functions/recordings/replay_extract_string.snap.sse
🚧 Files skipped from review as they are similar to previous changes (6)
- baml_language/sdk_tests/harness/llm_recordings/Cargo.toml
- baml_language/sdk_tests/crates/python_pydantic2/llm_functions/customizable/test_main.py
- baml_language/Cargo.toml
- baml_language/sdk_tests/harness/llm_recordings/tests/transport_pin.rs
- baml_language/sdk_tests/harness/llm_recordings/data/replay_server.baml
- baml_language/sdk_tests/harness/llm_recordings/tests/recordings.rs
Contributor
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
baml_language/crates/bex_engine/src/lib.rs (1)
1867-1902:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winSubstitute bound class type vars in
throws_typetoo.
run_entry_pointalso usesthrows_typeto type escaping throws. Leaving it generic preserves the same unbound-TypeVarfailure for methods whosethrowsclause mentions the receiver’s class type params.Apply the same bindings to `throws_type`
- let throws_type = self.function_throws_type(function_name); + let mut throws_type = self.function_throws_type(function_name); @@ crate::conversion::collect_type_var_bindings( self_declared, self_actual, &mut bindings, ); return_type = crate::conversion::substitute_type_vars(&return_type, &bindings); + if let Some(throws_type) = throws_type.as_mut() { + *throws_type = + crate::conversion::substitute_type_vars(throws_type, &bindings); + } } } @@ - let (mut return_type, throws_type, arity, param_types) = + let (mut return_type, mut throws_type, arity, param_types) = match thread.vm.get_object(func_ptr) { @@ for (declared, concrete) in declared_args.iter().zip(seed_type_args.iter()) { crate::conversion::collect_type_var_bindings(declared, concrete, &mut bindings); } return_type = crate::conversion::substitute_type_vars(&return_type, &bindings); + if let Some(throws_type) = throws_type.as_mut() { + *throws_type = + crate::conversion::substitute_type_vars(throws_type, &bindings); + } } }Also applies to: 2154-2192
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@baml_language/crates/bex_engine/src/lib.rs` around lines 1867 - 1902, The code collects type variable bindings from the receiver and applies them to the `return_type` through the `crate::conversion::substitute_type_vars` function, but fails to apply the same substitution to `throws_type`. This causes throws clauses that reference the receiver's class type parameters to remain unbound. Apply the same `crate::conversion::substitute_type_vars` substitution to `throws_type` using the collected `bindings` HashMap in the same conditional block where `return_type` is being substituted, ensuring both return and throws types are properly specialized.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@baml_language/crates/baml_builtins2/baml_std/baml/ns_http/http.baml`:
- Around line 113-116: The _timeout_nanos function collapses two distinct
timeout states into the same value: the null case returns 0n for "no timeout"
and a zero Duration also returns 0n when to_nanoseconds() is called, making
fetch or send with a zero timeout run unbounded instead of timing out
immediately. To fix this, change the null pattern match case in _timeout_nanos
to return a negative value (such as -1n) instead of 0n to represent "no
deadline", while allowing zero Duration objects to return 0n for an actual
zero-duration timeout. Then update the native timeout decoding logic that
interprets these values to treat negative values as "no deadline" and preserve 0
as a real zero-duration timeout.
In `@baml_language/crates/bex_engine/src/conversion.rs`:
- Around line 762-783: The Class branch in collect_type_var_bindings silently
processes mismatched classes without verifying they are the same class and have
the same arity, and the Union branch silently truncates when member counts
differ. Add guard checks in both branches to validate class identity and ensure
matching arity in the Class case (comparing the class identifiers and the
lengths of the type argument vectors), and ensure matching union member counts
in the Union case before proceeding with zip iteration. Add a unit test for
collect_type_var_bindings that covers both mismatched class scenarios and
mismatched union scenarios to verify the guards work correctly, then run cargo
test --lib to confirm all tests pass.
In
`@baml_language/sdk_tests/crates/python_pydantic2/llm_functions/customizable/replay_harness.py`:
- Line 76: After the thread.join(timeout=10) call in the teardown, add a
verification check to ensure the replay server thread has actually terminated.
If the thread is still alive after the timeout expires, raise an exception to
fail fast instead of silently continuing, which would allow a leaked daemon
server to potentially cause flaky tests. Use the thread's is_alive() method to
check termination status and handle the case where the shutdown stalls.
- Around line 67-80: The teardown code in the finally block unconditionally
deletes BAML_REPLAY_BASE_URL and BAML_REPLAY_API_KEY environment variables,
which corrupts test state if these variables were already set before entering
the harness. Save the original values of these environment variables before
setting them at the start of the context manager, and then restore them to their
original values in the finally block instead of unconditionally popping them. If
a variable had a prior value, restore it; if it didn't exist before, remove it
from the environment.
In
`@baml_language/sdk_tests/crates/typescript_node/llm_functions/customizable/replay_harness.ts`:
- Around line 43-55: The `withReplayServer` function unconditionally deletes the
replay environment variables without preserving their original values, which can
leak state across tests if those variables were preconfigured. Before setting
`process.env.BAML_REPLAY_BASE_URL` and `process.env.BAML_REPLAY_API_KEY`, store
their original values (if they exist). In the finally block, instead of using
delete operations on these variables, restore them to their original values (or
delete them only if they weren't previously set).
---
Outside diff comments:
In `@baml_language/crates/bex_engine/src/lib.rs`:
- Around line 1867-1902: The code collects type variable bindings from the
receiver and applies them to the `return_type` through the
`crate::conversion::substitute_type_vars` function, but fails to apply the same
substitution to `throws_type`. This causes throws clauses that reference the
receiver's class type parameters to remain unbound. Apply the same
`crate::conversion::substitute_type_vars` substitution to `throws_type` using
the collected `bindings` HashMap in the same conditional block where
`return_type` is being substituted, ensuring both return and throws types are
properly specialized.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ab6bbc4d-1d59-40a0-8de4-54d557a9e7fb
⛔ Files ignored due to path filters (14)
baml_language/Cargo.lockis excluded by!**/*.lockbaml_language/crates/baml_cli/src/snapshots/baml_cli__describe_command_tests__render_builtin_package_listing.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/__baml_std__/baml_tests__compiles____baml_std____03_hir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/__baml_std__/baml_tests__compiles____baml_std____04_5_mir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/__baml_std__/baml_tests__compiles____baml_std____06_codegen.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/src/compiler2_tir/snapshots/baml_tests__compiler2_tir__phase5__snapshot_baml_package_items.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/tests/bytecode_format/snapshots/bytecode_format__bytecode_display_expanded.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/tests/bytecode_format/snapshots/bytecode_format__bytecode_display_expanded_unoptimized.snapis excluded by!**/*.snapbaml_language/sdk_tests/fixtures/llm_functions/recordings/replay_extract_doc.snapis excluded by!**/*.snapbaml_language/sdk_tests/fixtures/llm_functions/recordings/replay_extract_string.snapis excluded by!**/*.snapbaml_language/sdks/nodejs/bridge_nodejs/dist/proto.d.ts.mapis excluded by!**/dist/**,!**/*.mapbaml_language/sdks/nodejs/bridge_nodejs/dist/proto.jsis excluded by!**/dist/**baml_language/sdks/nodejs/bridge_nodejs/dist/proto.js.mapis excluded by!**/dist/**,!**/*.mapbaml_language/sdks/python/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (28)
baml_language/Cargo.tomlbaml_language/crates/baml_builtins2/baml_std/baml/ns_http/http.bamlbaml_language/crates/baml_lsp_server/src/playground_http.rsbaml_language/crates/bex_engine/src/conversion.rsbaml_language/crates/bex_engine/src/lib.rsbaml_language/crates/bridge_wasm/src/wasm_http.rsbaml_language/crates/sys_native/src/http_server.rsbaml_language/crates/sys_native/src/io_impls.rsbaml_language/crates/sys_ops/src/lib.rsbaml_language/sdk_tests/crates/python_pydantic2/llm_functions/customizable/replay_harness.pybaml_language/sdk_tests/crates/python_pydantic2/llm_functions/customizable/test_main.pybaml_language/sdk_tests/crates/python_pydantic2/llm_functions/customizable/test_streaming_class_e2e.pybaml_language/sdk_tests/crates/python_pydantic2/llm_functions/customizable/test_streaming_e2e.pybaml_language/sdk_tests/crates/typescript_node/llm_functions/customizable/replay_harness.tsbaml_language/sdk_tests/crates/typescript_node/llm_functions/customizable/streaming_e2e.test.tsbaml_language/sdk_tests/fixtures/llm_functions/baml_src/ns_lorem/functions.bamlbaml_language/sdk_tests/fixtures/llm_functions/baml_src/ns_lorem/types.bamlbaml_language/sdk_tests/fixtures/llm_functions/baml_src/ns_replay/replay_server.bamlbaml_language/sdk_tests/fixtures/llm_functions/recordings/.gitignorebaml_language/sdk_tests/fixtures/llm_functions/recordings/README.mdbaml_language/sdk_tests/fixtures/llm_functions/recordings/replay_extract_doc.snap.ssebaml_language/sdk_tests/fixtures/llm_functions/recordings/replay_extract_string.snap.ssebaml_language/sdk_tests/harness/llm_recordings/Cargo.tomlbaml_language/sdk_tests/harness/llm_recordings/data/replay_server.bamlbaml_language/sdk_tests/harness/llm_recordings/src/lib.rsbaml_language/sdk_tests/harness/llm_recordings/tests/recordings.rsbaml_language/sdk_tests/harness/llm_recordings/tests/transport_pin.rsbaml_language/sdks/nodejs/bridge_nodejs/typescript_src/proto.ts
💤 Files with no reviewable changes (1)
- baml_language/sdk_tests/crates/python_pydantic2/llm_functions/customizable/test_streaming_class_e2e.py
✅ Files skipped from review due to trivial changes (6)
- baml_language/sdk_tests/fixtures/llm_functions/recordings/.gitignore
- baml_language/sdk_tests/fixtures/llm_functions/baml_src/ns_lorem/types.baml
- baml_language/sdk_tests/harness/llm_recordings/src/lib.rs
- baml_language/sdk_tests/fixtures/llm_functions/recordings/README.md
- baml_language/sdk_tests/fixtures/llm_functions/recordings/replay_extract_doc.snap.sse
- baml_language/sdk_tests/fixtures/llm_functions/recordings/replay_extract_string.snap.sse
🚧 Files skipped from review as they are similar to previous changes (6)
- baml_language/sdk_tests/harness/llm_recordings/Cargo.toml
- baml_language/sdk_tests/crates/python_pydantic2/llm_functions/customizable/test_main.py
- baml_language/Cargo.toml
- baml_language/sdk_tests/harness/llm_recordings/tests/transport_pin.rs
- baml_language/sdk_tests/harness/llm_recordings/data/replay_server.baml
- baml_language/sdk_tests/harness/llm_recordings/tests/recordings.rs
🛑 Comments failed to post (5)
baml_language/crates/baml_builtins2/baml_std/baml/ns_http/http.baml (1)
113-116:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winZero-duration timeouts are silently converted into “no timeout.”
Line [115] encodes
nullas0n, but Line [116] also yields0nwhen callers pass a zeroDuration. That collapses two distinct states and can makefetch(..., 0)/send(..., 0)run unbounded instead of timing out immediately.Suggested contract-safe direction
function _timeout_nanos(timeout: root.time.Duration?) -> bigint throws never { match (timeout) { - null => 0n, + null => -1n, let t: root.time.Duration => t.to_nanoseconds(), } }Then update native timeout decoding to interpret
< 0as “no deadline,” preserving0as a real zero-duration timeout.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@baml_language/crates/baml_builtins2/baml_std/baml/ns_http/http.baml` around lines 113 - 116, The _timeout_nanos function collapses two distinct timeout states into the same value: the null case returns 0n for "no timeout" and a zero Duration also returns 0n when to_nanoseconds() is called, making fetch or send with a zero timeout run unbounded instead of timing out immediately. To fix this, change the null pattern match case in _timeout_nanos to return a negative value (such as -1n) instead of 0n to represent "no deadline", while allowing zero Duration objects to return 0n for an actual zero-duration timeout. Then update the native timeout decoding logic that interprets these values to treat negative values as "no deadline" and preserve 0 as a real zero-duration timeout.baml_language/crates/bex_engine/src/conversion.rs (1)
762-783:
⚠️ Potential issue | 🟡 Minor🧩 Analysis chain
🏁 Script executed:
# First, let's examine the file and locate the function cat -n baml_language/crates/bex_engine/src/conversion.rs | sed -n '740,800p'Repository: BoundaryML/baml
Length of output: 3295
🏁 Script executed:
# Search for the function definition rg -n "fn collect_type_var_bindings" baml_language/crates/bex_engine/src/conversion.rs -A 50Repository: BoundaryML/baml
Length of output: 2388
🏁 Script executed:
# Check if there are tests in the same file or test modules rg -n "#\[test\]|#\[cfg\(test\)\]" baml_language/crates/bex_engine/src/conversion.rsRepository: BoundaryML/baml
Length of output: 435
🏁 Script executed:
# Look for test files related to conversion fd "test" baml_language/crates/bex_engine/src/ | head -20Repository: BoundaryML/baml
Length of output: 41
🏁 Script executed:
# Check what RuntimeTy looks like to understand the structure rg -n "enum RuntimeTy|struct RuntimeTy" baml_language/crates/bex_engine/src/ -A 10Repository: BoundaryML/baml
Length of output: 41
🏁 Script executed:
# Search for RuntimeTy definition rg "enum RuntimeTy" baml_language/crates/bex_engine/src/ -A 40 | head -80Repository: BoundaryML/baml
Length of output: 41
🏁 Script executed:
# Check what calls collect_type_var_bindings and how it's used rg "collect_type_var_bindings" baml_language/crates/bex_engine/src/ -B 2 -A 2Repository: BoundaryML/baml
Length of output: 4848
🏁 Script executed:
# Look at the test modules to understand what is tested sed -n '1723,1800p' baml_language/crates/bex_engine/src/conversion.rsRepository: BoundaryML/baml
Length of output: 2780
🏁 Script executed:
# Check the comment about intended use and understand the logic sed -n '740,790p' baml_language/crates/bex_engine/src/conversion.rsRepository: BoundaryML/baml
Length of output: 2255
🏁 Script executed:
# Look for RuntimeTy::Class definition to understand its structure rg "Class\(" baml_language/crates/bex_engine/src/ -B 2 -A 2 | head -40Repository: BoundaryML/baml
Length of output: 3149
🏁 Script executed:
# Check if there are type checking constraints before calling collect_type_var_bindings sed -n '1,100p' baml_language/crates/bex_engine/src/lib.rs | grep -A 20 "collect_type_var_bindings"Repository: BoundaryML/baml
Length of output: 41
🏁 Script executed:
# Look for where seed_type_args and declared_args come from in lib.rs rg "seed_type_args|declared_args" baml_language/crates/bex_engine/src/lib.rs -B 5 -A 2Repository: BoundaryML/baml
Length of output: 1605
🏁 Script executed:
# Check if there are any guards in the calling code before collect_type_var_bindings is invoked rg -B 10 "collect_type_var_bindings" baml_language/crates/bex_engine/src/lib.rs | head -60Repository: BoundaryML/baml
Length of output: 1648
🏁 Script executed:
# Search for any existing tests of collect_type_var_bindings rg -n "collect_type_var_bindings" baml_language/crates/bex_engine/src/conversion.rs | grep -i testRepository: BoundaryML/baml
Length of output: 41
🏁 Script executed:
# Look at the test modules at the end of conversion.rs to see what functions are tested sed -n '1847,2000p' baml_language/crates/bex_engine/src/conversion.rsRepository: BoundaryML/baml
Length of output: 4673
🏁 Script executed:
# Check substitute_type_vars to understand the full flow rg -n "fn substitute_type_vars" baml_language/crates/bex_engine/src/conversion.rs -A 30Repository: BoundaryML/baml
Length of output: 1274
🏁 Script executed:
# Verify if there are any assertions or validation of class matching before collect_type_var_bindings is called rg -B 20 "collect_type_var_bindings" baml_language/crates/bex_engine/src/lib.rs | grep -A 20 "self_declared\|declared_args"Repository: BoundaryML/baml
Length of output: 1420
🏁 Script executed:
# Look at where self_declared comes from and what guarantees exist rg -B 30 "self_declared" baml_language/crates/bex_engine/src/lib.rs | head -80Repository: BoundaryML/baml
Length of output: 2153
🏁 Script executed:
# Check how param_types is constructed to see if there are any type-checking constraints rg -n "param_types" baml_language/crates/bex_engine/src/lib.rs -B 3 -A 1 | head -40Repository: BoundaryML/baml
Length of output: 1893
🏁 Script executed:
# Look for RuntimeTy definition to understand Class structure fd RuntimeTy baml_language/crates/ | head -5Repository: BoundaryML/baml
Length of output: 41
🏁 Script executed:
# Find the actual RuntimeTy enum definition rg "enum RuntimeTy" baml_language/crates/ -A 50 | head -100Repository: BoundaryML/baml
Length of output: 4326
🏁 Script executed:
# Check what function_params returns and how it's guaranteed to match rg -n "function_params" baml_language/crates/bex_engine/src/lib.rs -A 5 | head -40Repository: BoundaryML/baml
Length of output: 1097
🏁 Script executed:
# Look at the definition of tagged_handle_runtime_ty to understand what it returns rg -n "fn tagged_handle_runtime_ty" baml_language/crates/bex_engine/src/conversion.rs -A 20Repository: BoundaryML/baml
Length of output: 1209
🏁 Script executed:
# Check if there are any assertions or validation when creating Class instances rg "RuntimeTy::Class\(" baml_language/crates/bex_engine/src/ -B 2 -A 2 | head -60Repository: BoundaryML/baml
Length of output: 5259
🏁 Script executed:
# Look at validate_bound_args to see if it ensures class matching rg -n "fn validate_bound_args" baml_language/crates/bex_engine/src/lib.rs -A 30Repository: BoundaryML/baml
Length of output: 1293
🏁 Script executed:
# Check what happens when classes don't match - is there any error handling upstream? rg -n "TypeMismatch|class.*mismatch|type.*mismatch" baml_language/crates/bex_engine/src/lib.rs -i | head -20Repository: BoundaryML/baml
Length of output: 586
🏁 Script executed:
# Verify the context where collect_type_var_bindings is called to understand if there's validation sed -n '1880,1910p' baml_language/crates/bex_engine/src/lib.rsRepository: BoundaryML/baml
Length of output: 1748
🏁 Script executed:
# Check the run_entry_point context more carefully rg -n "fn run_entry_point" baml_language/crates/bex_engine/src/lib.rs -A 50 | head -80Repository: BoundaryML/baml
Length of output: 2485
🏁 Script executed:
# Check if there's any type validation when tagged_handle_runtime_ty is used rg -B 10 "tagged_handle_runtime_ty" baml_language/crates/bex_engine/src/lib.rs | grep -A 10 "self_declared"Repository: BoundaryML/baml
Length of output: 293
🏁 Script executed:
# Look for how the self_actual is obtained and if there are any guarantees sed -n '1895,1905p' baml_language/crates/bex_engine/src/lib.rsRepository: BoundaryML/baml
Length of output: 498
🏁 Script executed:
# Check if there are existing tests in conversion.rs sed -n '1723,1850p' baml_language/crates/bex_engine/src/conversion.rsRepository: BoundaryML/baml
Length of output: 4072
🏁 Script executed:
# Examine if collect_type_var_bindings is currently tested rg "collect_type_var_bindings" baml_language/crates/bex_engine/src/conversion.rs -A 2 -B 2 | grep -i testRepository: BoundaryML/baml
Length of output: 41
Add guards for class identity and aggregate arity in
collect_type_var_bindings.The Class branch (line 762) ignores class names and relies on
zipto handle arity, silently binding type vars from mismatched classes. Similarly, the Union branch (line 779) silently truncates on member-count mismatch. Without identity/arity checks, a mismatched or malformed receiver can causesubstitute_type_varsto rewrite the return type incorrectly.Add the following guards:
Suggested changes
- (RuntimeTy::Class(_, da, _), RuntimeTy::Class(_, ca, _)) => { - for (d, c) in da.iter().zip(ca.iter()) { - collect_type_var_bindings(d, c, out); + (RuntimeTy::Class(dn, da, _), RuntimeTy::Class(cn, ca, _)) => { + if dn == cn && da.len() == ca.len() { + for (d, c) in da.iter().zip(ca.iter()) { + collect_type_var_bindings(d, c, out); + } } } @@ - (RuntimeTy::Union(dm, _), RuntimeTy::Union(cm, _)) => { - for (d, c) in dm.iter().zip(cm.iter()) { - collect_type_var_bindings(d, c, out); + (RuntimeTy::Union(dm, _), RuntimeTy::Union(cm, _)) => { + if dm.len() == cm.len() { + for (d, c) in dm.iter().zip(cm.iter()) { + collect_type_var_bindings(d, c, out); + } } }Per the coding guidelines for
**/*.rsfiles, add a unit test for this function and runcargo test --libto confirm all tests pass.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@baml_language/crates/bex_engine/src/conversion.rs` around lines 762 - 783, The Class branch in collect_type_var_bindings silently processes mismatched classes without verifying they are the same class and have the same arity, and the Union branch silently truncates when member counts differ. Add guard checks in both branches to validate class identity and ensure matching arity in the Class case (comparing the class identifiers and the lengths of the type argument vectors), and ensure matching union member counts in the Union case before proceeding with zip iteration. Add a unit test for collect_type_var_bindings that covers both mismatched class scenarios and mismatched union scenarios to verify the guards work correctly, then run cargo test --lib to confirm all tests pass.Source: Coding guidelines
baml_language/sdk_tests/crates/python_pydantic2/llm_functions/customizable/replay_harness.py (2)
67-80:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winRestore prior replay env values instead of always deleting them
This teardown clears
BAML_REPLAY_BASE_URL/BAML_REPLAY_API_KEYunconditionally, which can corrupt surrounding test state when these vars were already set before entering the harness.Proposed fix
def _running_server(recording: str): @@ - os.environ["BAML_REPLAY_BASE_URL"] = f"http://{addr}" - os.environ["BAML_REPLAY_API_KEY"] = "replay-test-key" + prev_base_url = os.environ.get("BAML_REPLAY_BASE_URL") + prev_api_key = os.environ.get("BAML_REPLAY_API_KEY") + os.environ["BAML_REPLAY_BASE_URL"] = f"http://{addr}" + os.environ["BAML_REPLAY_API_KEY"] = "replay-test-key" @@ - for key in ("BAML_REPLAY_BASE_URL", "BAML_REPLAY_API_KEY"): - os.environ.pop(key, None) + if prev_base_url is None: + os.environ.pop("BAML_REPLAY_BASE_URL", None) + else: + os.environ["BAML_REPLAY_BASE_URL"] = prev_base_url + + if prev_api_key is None: + os.environ.pop("BAML_REPLAY_API_KEY", None) + else: + os.environ["BAML_REPLAY_API_KEY"] = prev_api_key📝 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.prev_base_url = os.environ.get("BAML_REPLAY_BASE_URL") prev_api_key = os.environ.get("BAML_REPLAY_API_KEY") os.environ["BAML_REPLAY_BASE_URL"] = f"http://{addr}" os.environ["BAML_REPLAY_API_KEY"] = "replay-test-key" try: yield addr finally: with contextlib.suppress(Exception): urllib.request.urlopen( f"http://{addr}/__replay__/shutdown", data=b"", timeout=5 ) thread.join(timeout=10) with contextlib.suppress(FileNotFoundError): addr_file.unlink() if prev_base_url is None: os.environ.pop("BAML_REPLAY_BASE_URL", None) else: os.environ["BAML_REPLAY_BASE_URL"] = prev_base_url if prev_api_key is None: os.environ.pop("BAML_REPLAY_API_KEY", None) else: os.environ["BAML_REPLAY_API_KEY"] = prev_api_key🧰 Tools
🪛 ast-grep (0.43.0)
[warning] 73-73: Do not make http calls without encryption
Context: f"http://{addr}/replay/shutdown"
Note: [CWE-319].(requests-http)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@baml_language/sdk_tests/crates/python_pydantic2/llm_functions/customizable/replay_harness.py` around lines 67 - 80, The teardown code in the finally block unconditionally deletes BAML_REPLAY_BASE_URL and BAML_REPLAY_API_KEY environment variables, which corrupts test state if these variables were already set before entering the harness. Save the original values of these environment variables before setting them at the start of the context manager, and then restore them to their original values in the finally block instead of unconditionally popping them. If a variable had a prior value, restore it; if it didn't exist before, remove it from the environment.
76-76:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winFail fast if replay server thread does not actually stop
After timed join, teardown does not verify thread termination. If shutdown stalls, a leaked daemon server can make later tests flaky.
Proposed fix
- thread.join(timeout=10) + thread.join(timeout=10) + if thread.is_alive(): + raise TimeoutError("replay server thread did not shut down within 10s")📝 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.thread.join(timeout=10) if thread.is_alive(): raise TimeoutError("replay server thread did not shut down within 10s")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@baml_language/sdk_tests/crates/python_pydantic2/llm_functions/customizable/replay_harness.py` at line 76, After the thread.join(timeout=10) call in the teardown, add a verification check to ensure the replay server thread has actually terminated. If the thread is still alive after the timeout expires, raise an exception to fail fast instead of silently continuing, which would allow a leaked daemon server to potentially cause flaky tests. Use the thread's is_alive() method to check termination status and handle the case where the shutdown stalls.baml_language/sdk_tests/crates/typescript_node/llm_functions/customizable/replay_harness.ts (1)
43-55:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve and restore prior replay env values in teardown
withReplayServeralways deletes replay env vars, which can leak state across tests when those vars were preconfigured before this wrapper executes.Proposed fix
export function withReplayServer( recording: string, body: () => void | Promise<void>, ): () => Promise<void> { return async () => { const addr = replay_serve_detached(recordingPath(recording)); + const prevBaseUrl = process.env.BAML_REPLAY_BASE_URL; + const prevApiKey = process.env.BAML_REPLAY_API_KEY; process.env.BAML_REPLAY_BASE_URL = `http://${addr}`; process.env.BAML_REPLAY_API_KEY = "replay-test-key"; try { await body(); } finally { @@ - delete process.env.BAML_REPLAY_BASE_URL; - delete process.env.BAML_REPLAY_API_KEY; + if (prevBaseUrl === undefined) { + delete process.env.BAML_REPLAY_BASE_URL; + } else { + process.env.BAML_REPLAY_BASE_URL = prevBaseUrl; + } + if (prevApiKey === undefined) { + delete process.env.BAML_REPLAY_API_KEY; + } else { + process.env.BAML_REPLAY_API_KEY = prevApiKey; + } } }; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@baml_language/sdk_tests/crates/typescript_node/llm_functions/customizable/replay_harness.ts` around lines 43 - 55, The `withReplayServer` function unconditionally deletes the replay environment variables without preserving their original values, which can leak state across tests if those variables were preconfigured. Before setting `process.env.BAML_REPLAY_BASE_URL` and `process.env.BAML_REPLAY_API_KEY`, store their original values (if they exist). In the finally block, instead of using delete operations on these variables, restore them to their original values (or delete them only if they weren't previously set).
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@baml_language/sdk_tests/crates/python_pydantic2/llm_functions/customizable/replay_harness.py`:
- Around line 52-80: The cleanup block in the finally clause will be skipped if
an exception occurs before the yield statement (such as in the deadline loop or
at the TimeoutError), leaving the server thread and temp file behind.
Additionally, the environment variable cleanup using os.environ.pop discards
pre-existing values instead of restoring them. Store the original values of
"BAML_REPLAY_BASE_URL" and "BAML_REPLAY_API_KEY" before setting them, and
restore these saved values in the finally block instead of using pop. This
ensures cleanup always runs and environment variables are properly restored to
their original state regardless of when an exception occurs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 0ccce4a7-1209-41fc-a90e-78b9256115cd
⛔ Files ignored due to path filters (1)
baml_language/sdks/python/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
baml_language/sdk_tests/crates/python_pydantic2/llm_functions/customizable/replay_harness.pybaml_language/sdk_tests/crates/python_pydantic2/llm_functions/customizable/test_main.pybaml_language/sdk_tests/crates/python_pydantic2/llm_functions/customizable/test_streaming_class_e2e.pybaml_language/sdk_tests/crates/python_pydantic2/llm_functions/customizable/test_streaming_e2e.pybaml_language/sdk_tests/crates/typescript_node/llm_functions/customizable/replay_harness.tsbaml_language/sdk_tests/crates/typescript_node/llm_functions/customizable/streaming_e2e.test.tsbaml_language/sdk_tests/fixtures/llm_functions/baml_src/ns_lorem/functions.bamlbaml_language/sdk_tests/fixtures/llm_functions/baml_src/ns_lorem/types.bamlbaml_language/sdk_tests/fixtures/llm_functions/baml_src/ns_replay/replay_server.baml
💤 Files with no reviewable changes (1)
- baml_language/sdk_tests/crates/python_pydantic2/llm_functions/customizable/test_streaming_class_e2e.py
✅ Files skipped from review due to trivial changes (1)
- baml_language/sdk_tests/fixtures/llm_functions/baml_src/ns_lorem/types.baml
🚧 Files skipped from review as they are similar to previous changes (6)
- baml_language/sdk_tests/crates/python_pydantic2/llm_functions/customizable/test_main.py
- baml_language/sdk_tests/fixtures/llm_functions/baml_src/ns_replay/replay_server.baml
- baml_language/sdk_tests/crates/python_pydantic2/llm_functions/customizable/test_streaming_e2e.py
- baml_language/sdk_tests/crates/typescript_node/llm_functions/customizable/replay_harness.ts
- baml_language/sdk_tests/crates/typescript_node/llm_functions/customizable/streaming_e2e.test.ts
- baml_language/sdk_tests/fixtures/llm_functions/baml_src/ns_lorem/functions.baml
Comment on lines
+52
to
+80
| thread = threading.Thread(target=_serve, daemon=True) | ||
| thread.start() | ||
|
|
||
| addr = None | ||
| deadline = time.time() + 10.0 | ||
| while time.time() < deadline: | ||
| if error: | ||
| raise RuntimeError(f"replay server thread failed: {error[0]!r}") | ||
| if addr_file.exists() and (text := addr_file.read_text().strip()): | ||
| addr = text | ||
| break | ||
| time.sleep(0.02) | ||
| if addr is None: | ||
| raise TimeoutError("replay server did not bind within 10s") | ||
|
|
||
| os.environ["BAML_REPLAY_BASE_URL"] = f"http://{addr}" | ||
| os.environ["BAML_REPLAY_API_KEY"] = "replay-test-key" | ||
| try: | ||
| yield addr | ||
| finally: | ||
| with contextlib.suppress(Exception): | ||
| urllib.request.urlopen( | ||
| f"http://{addr}/__replay__/shutdown", data=b"", timeout=5 | ||
| ) | ||
| thread.join(timeout=10) | ||
| with contextlib.suppress(FileNotFoundError): | ||
| addr_file.unlink() | ||
| for key in ("BAML_REPLAY_BASE_URL", "BAML_REPLAY_API_KEY"): | ||
| os.environ.pop(key, None) |
Contributor
There was a problem hiding this comment.
Ensure teardown runs on startup failure and restore prior env values.
Line 59 and Line 65 can raise before yield, so the cleanup block at Line 72-80 is skipped; that can leave the server thread/temp file behind. Also, popping env keys at Line 79-80 discards any pre-existing values instead of restoring them.
Proposed fix
@@
- thread = threading.Thread(target=_serve, daemon=True)
- thread.start()
-
- addr = None
- deadline = time.time() + 10.0
- while time.time() < deadline:
- if error:
- raise RuntimeError(f"replay server thread failed: {error[0]!r}")
- if addr_file.exists() and (text := addr_file.read_text().strip()):
- addr = text
- break
- time.sleep(0.02)
- if addr is None:
- raise TimeoutError("replay server did not bind within 10s")
-
- os.environ["BAML_REPLAY_BASE_URL"] = f"http://{addr}"
- os.environ["BAML_REPLAY_API_KEY"] = "replay-test-key"
- try:
- yield addr
- finally:
- with contextlib.suppress(Exception):
- urllib.request.urlopen(
- f"http://{addr}/__replay__/shutdown", data=b"", timeout=5
- )
- thread.join(timeout=10)
- with contextlib.suppress(FileNotFoundError):
- addr_file.unlink()
- for key in ("BAML_REPLAY_BASE_URL", "BAML_REPLAY_API_KEY"):
- os.environ.pop(key, None)
+ thread = threading.Thread(target=_serve, daemon=True)
+ thread.start()
+ addr = None
+ previous_env = {
+ "BAML_REPLAY_BASE_URL": os.environ.get("BAML_REPLAY_BASE_URL"),
+ "BAML_REPLAY_API_KEY": os.environ.get("BAML_REPLAY_API_KEY"),
+ }
+ try:
+ deadline = time.time() + 10.0
+ while time.time() < deadline:
+ if error:
+ raise RuntimeError(f"replay server thread failed: {error[0]!r}")
+ if addr_file.exists() and (text := addr_file.read_text().strip()):
+ addr = text
+ break
+ time.sleep(0.02)
+ if addr is None:
+ raise TimeoutError("replay server did not bind within 10s")
+
+ os.environ["BAML_REPLAY_BASE_URL"] = f"http://{addr}"
+ os.environ["BAML_REPLAY_API_KEY"] = "replay-test-key"
+ yield addr
+ finally:
+ if addr is not None:
+ with contextlib.suppress(Exception):
+ urllib.request.urlopen(
+ f"http://{addr}/__replay__/shutdown", data=b"", timeout=5
+ )
+ thread.join(timeout=10)
+ with contextlib.suppress(FileNotFoundError):
+ addr_file.unlink()
+ for key, old_value in previous_env.items():
+ if old_value is None:
+ os.environ.pop(key, None)
+ else:
+ os.environ[key] = old_value📝 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
| thread = threading.Thread(target=_serve, daemon=True) | |
| thread.start() | |
| addr = None | |
| deadline = time.time() + 10.0 | |
| while time.time() < deadline: | |
| if error: | |
| raise RuntimeError(f"replay server thread failed: {error[0]!r}") | |
| if addr_file.exists() and (text := addr_file.read_text().strip()): | |
| addr = text | |
| break | |
| time.sleep(0.02) | |
| if addr is None: | |
| raise TimeoutError("replay server did not bind within 10s") | |
| os.environ["BAML_REPLAY_BASE_URL"] = f"http://{addr}" | |
| os.environ["BAML_REPLAY_API_KEY"] = "replay-test-key" | |
| try: | |
| yield addr | |
| finally: | |
| with contextlib.suppress(Exception): | |
| urllib.request.urlopen( | |
| f"http://{addr}/__replay__/shutdown", data=b"", timeout=5 | |
| ) | |
| thread.join(timeout=10) | |
| with contextlib.suppress(FileNotFoundError): | |
| addr_file.unlink() | |
| for key in ("BAML_REPLAY_BASE_URL", "BAML_REPLAY_API_KEY"): | |
| os.environ.pop(key, None) | |
| thread = threading.Thread(target=_serve, daemon=True) | |
| thread.start() | |
| addr = None | |
| previous_env = { | |
| "BAML_REPLAY_BASE_URL": os.environ.get("BAML_REPLAY_BASE_URL"), | |
| "BAML_REPLAY_API_KEY": os.environ.get("BAML_REPLAY_API_KEY"), | |
| } | |
| try: | |
| deadline = time.time() + 10.0 | |
| while time.time() < deadline: | |
| if error: | |
| raise RuntimeError(f"replay server thread failed: {error[0]!r}") | |
| if addr_file.exists() and (text := addr_file.read_text().strip()): | |
| addr = text | |
| break | |
| time.sleep(0.02) | |
| if addr is None: | |
| raise TimeoutError("replay server did not bind within 10s") | |
| os.environ["BAML_REPLAY_BASE_URL"] = f"http://{addr}" | |
| os.environ["BAML_REPLAY_API_KEY"] = "replay-test-key" | |
| yield addr | |
| finally: | |
| if addr is not None: | |
| with contextlib.suppress(Exception): | |
| urllib.request.urlopen( | |
| f"http://{addr}/__replay__/shutdown", data=b"", timeout=5 | |
| ) | |
| thread.join(timeout=10) | |
| with contextlib.suppress(FileNotFoundError): | |
| addr_file.unlink() | |
| for key, old_value in previous_env.items(): | |
| if old_value is None: | |
| os.environ.pop(key, None) | |
| else: | |
| os.environ[key] = old_value |
🧰 Tools
🪛 ast-grep (0.43.0)
[warning] 66-66: Do not make http calls without encryption
Context: f"http://{addr}"
Note: [CWE-319].
(requests-http)
[warning] 73-73: Do not make http calls without encryption
Context: f"http://{addr}/replay/shutdown"
Note: [CWE-319].
(requests-http)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@baml_language/sdk_tests/crates/python_pydantic2/llm_functions/customizable/replay_harness.py`
around lines 52 - 80, The cleanup block in the finally clause will be skipped if
an exception occurs before the yield statement (such as in the deadline loop or
at the TimeoutError), leaving the server thread and temp file behind.
Additionally, the environment variable cleanup using os.environ.pop discards
pre-existing values instead of restoring them. Store the original values of
"BAML_REPLAY_BASE_URL" and "BAML_REPLAY_API_KEY" before setting them, and
restore these saved values in the finally block instead of using pop. This
ensures cleanup always runs and environment variables are properly restored to
their original state regardless of when an exception occurs.
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.
sdk_test_llm_recordingsthat allow powering streaming tests with recorded tracesSummary by CodeRabbit
Release Notes
New Features
new_streaming(),write(), andend()for chunked server output.Tests
Bug Fixes
Chores