Add streaming response support: client, server, and concurrent handlers - #641
Conversation
…xt chunk|line)
Add generic outbound response streaming to the WFL client so a large or
progressively-emitted upstream body can be consumed without buffering:
open url at "<url>" [with method .. and headers .. and body ..]
and stream response as upstream
wait for next line from upstream as line // Text, or nothing at EOF
wait for next chunk from upstream as chunk // Binary, or nothing at EOF
close upstream // cancels the upstream request
`stream response as` returns as soon as the status/headers arrive, without
buffering the body, binding an object with status/ok/headers and an internal
stream id. The body is parked in a new IoClient stream-handle table and pulled
incrementally; each read takes the handle out of the map so a slow read never
blocks other streams.
Lifecycle (client side): the head phase and every per-chunk read go through the
existing run_http_with_budget select, so connect/read timeouts, the
response-byte ceiling (enforced on the running total, not just Content-Length),
and cooperative cancellation all apply. Mid-stream network errors are catchable
RuntimeErrors; dropping the handle (EOF, error, close, teardown) cancels the
upstream; reading a closed/drained handle is a predictable error. A final
unterminated line is delivered before a single clean `nothing` at EOF.
- AST: HttpStreamStatement, WaitForNextChunkStatement, WaitForNextLineStatement
- Parser: `stream response as` clause (io.rs); `wait for next chunk|line from`
(processes.rs); no new lexer tokens (contextual identifiers)
- Interpreter: IoClient open_http_stream/next_chunk/next_line/close_stream;
three statement arms; `close` extended to accept a streaming-response object
- Analyzer/typechecker/transpiler: variable-binding registration; explicit
unsupported-in-JS-transpilation arm
- Docs: interoperability.md "Streaming a response incrementally" + validated
example; Dev Diary entry
- Tests: tests/http_stream_test.rs (parser + offline runtime, incl. EOF and
closed-stream error)
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011LxXAm5E4Vy39kRLr8kRnX
…g roadmap
Capture the turnkey design for server-side response streaming and the overall
response-streaming roadmap, so the next increment is unambiguous:
- Chosen surface: `start streaming response to <req> with status .. and content
type .. as <out>`, `write line|chunk <expr> to <out>`, `flush <out>`,
`close <out>` — consistent with the shipped client streaming and dispatched
like the existing `send websocket message` leading-identifier statements.
- Mechanism: a HandlerReply enum (Buffered | Streaming{status, content_type,
headers, body: mpsc::Receiver}) over the existing per-request oneshot; the
warp route's final closure converts to a Body-typed reply (Body::from for the
buffered/504 arms, Body::wrap_stream(unfold(rx)) for the streaming arm) while
the recover helper stays Vec<u8> (warp unifies via Either) — so only the final
handler closure changes, not the five reply helpers.
- Lifecycle: bounded mpsc for backpressure; a closed receiver (browser
disconnect) makes `write` fail catchably, which the handler uses to close the
upstream; `close`/drop ends the body; writes after close fail predictably.
Also records status of all five requested capabilities: items 1-2 and the
client half of item 5 shipped; item 3 designed; item 4 deferred to the locked
concurrency-phase-plan (Phase 1 `main loop concurrently:`).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011LxXAm5E4Vy39kRLr8kRnX
…e|chunk / flush / close)
Add server-side response streaming so a WFL handler can send status/headers
immediately and produce the body progressively, without buffering:
start streaming response to req with status 200 and content type "application/x-ndjson" as out
write line json_text to out // frames a line (newline appended)
write chunk raw_bytes to out // raw bytes/text, verbatim
flush out // advisory
close out // ends the response body
Combined with the client-side `stream response as upstream` + `wait for next
line`, a handler can proxy a slow upstream to the browser line-by-line.
Mechanism:
- The per-request oneshot now carries a HandlerReply enum
(Buffered(WflHttpResponse) | Streaming{status, content_type, headers,
body: mpsc::Receiver}). `respond` sends Buffered; `start streaming response`
sends Streaming with a bounded body channel's receiver.
- The warp route's final closure returns a Body-typed reply. warp's recover
unifies reply types via Either, so only that closure changed: it wraps the
buffered/504 arms with Body::from and builds the streaming arm with
Body::wrap_stream(unfold(rx)) (no new dependency); the five Vec<u8> reply
helpers are untouched.
- Backpressure: the body channel is bounded (64) so a slow client backpressures
`write`. Disconnect: hyper drops the body -> receiver dropped -> the next
`write` fails with a catchable error, propagating the disconnect to the
handler (which can close any upstream it proxies). `close`/handler-exit drops
the sender, ending the response.
- `start` is a keyword; `streaming`/`flush`/`line`/`chunk` are identifiers.
`close` now also closes a server response stream (`_server_stream`).
- AST: StartStreamingResponseStatement, StreamWriteStatement, FlushStreamStatement
- Parser: parse_start_streaming_response/parse_flush_stream (web.rs), write
line|chunk branch (io.rs), KeywordStart/flush dispatch (parser/mod.rs)
- Interpreter: server_response_streams map, three exec arms, close extension
- Analyzer/typechecker/transpiler arms
- Docs: web-servers.md "Streaming a response" (+ upstream-proxy example);
response-streaming-design.md status; validated example; Dev Diary
- Tests: tests/http_server_streaming_test.rs (parser + end-to-end streaming
server read back with reqwest); existing web-server suites still green
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011LxXAm5E4Vy39kRLr8kRnX
Add opt-in cooperative concurrent request handling so a slow handler (e.g. one
proxying a slow upstream stream) no longer blocks other requests:
main loop concurrently:
wait for request comes in on server as req
... slow handler ...
respond to req with "..."
end loop
Plain `main loop` stays strictly serial and byte-compatible; `concurrently` is
the only opt-in and nothing changes silently. This is Phase 1 of the
maintainer-locked concurrency-phase-plan.md (locked marker, no Rc->Arc/Send
rewrite of the interpreter core) and lands at that plan's STOP/review gate.
Mechanism:
- AST: MainLoop gains `concurrent: bool`. `concurrently` is a contextual
identifier (only special right after `main loop`), so it stays usable as a
variable name elsewhere.
- execute_concurrent_main_loop keeps up to CONCURRENT_HANDLER_LIMIT (256) body
iterations in flight via a FuturesUnordered of !Send, non-'static,
&self-borrowing handler futures — cooperative concurrency on the single
interpreter thread (no spawn_local, no Send/Arc across the core). Each
iteration runs in a fresh isolated child scope. Cap >= 1 keeps the set
non-empty, avoiding the Ready(None) busy-spin trap.
- Containment: each handler future is AssertUnwindSafe(...).catch_unwind(); a
panicking or erroring handler is contained and its request is answered 500 by
the existing ResponseCompletion drop guard while siblings keep running.
- 503/504/500 are inherited from the existing transport layer (bounded queue,
response deadline, drop guard); `wait for request` releases the receiver lock
before the handler runs, so iterations hand off requests one at a time then
handle them concurrently. No RefCell held across await (crate-wide
await_holding_refcell_ref deny enforces this).
- Docs: web-servers.md "Concurrent request handling" (concurrent != parallel,
yield cliff, 503/504/500) + validated example; concurrency-phase-plan.md
tracker updated to Phase-1-done/awaiting-review; Dev Diary.
- Tests: tests/concurrent_main_loop_test.rs — concurrently parses vs serial;
slow handler does not block fast sibling; serial DOES block (no silent
upgrade); handler-error containment keeps the server serving.
Also removes two .ast.txt parse-dump artifacts accidentally added in the two
prior streaming commits.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011LxXAm5E4Vy39kRLr8kRnX
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe interpreter adds outbound response streaming, server-side streamed responses, and opt-in cooperative concurrency for HTTP request handlers. New WFL syntax is parsed, analyzed, type-checked, documented, demonstrated, and covered by parser and runtime tests. ChangesHTTP streaming and concurrent handlers
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant WFL
participant IoClient
participant Upstream
WFL->>IoClient: open url and stream response
IoClient->>Upstream: send HTTP request
Upstream-->>IoClient: return status and headers
IoClient-->>WFL: bind streaming handle
WFL->>IoClient: wait for next line or chunk
IoClient-->>WFL: return body data or nothing
sequenceDiagram
participant Handler
participant StreamChannel
participant WarpTransport
participant HTTPClient
Handler->>WarpTransport: start streaming response
WarpTransport->>StreamChannel: receive bounded body channel
Handler->>StreamChannel: write line or chunk
StreamChannel-->>HTTPClient: deliver body data
Handler->>StreamChannel: close response
StreamChannel-->>HTTPClient: end response body
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ed61ba5ac7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| futs.push( | ||
| std::panic::AssertUnwindSafe(self.execute_block(body, scope)).catch_unwind(), | ||
| ); |
There was a problem hiding this comment.
When main loop concurrently: runs multiple execute_block futures on the same Interpreter, the child environments are isolated but interpreter-level state is not. For example, a handler that awaits inside a count loop can be resumed while another handler has overwritten self.current_count/self.in_count_loop, and count lookup reads those shared fields before the environment binding, so concurrent requests can return each other's loop counters or leave stale loop state behind.
Useful? React with 👍 / 👎.
| self.server_response_streams | ||
| .borrow_mut() | ||
| .insert(handle_id.clone(), tx); |
There was a problem hiding this comment.
After start streaming response, the only sender that can close the HTTP body is stored in server_response_streams, but there is no per-handler cleanup when the iteration returns or errors. In a long-running main loop/main loop concurrently: handler that writes a few chunks and then falls off the end (or errors after the stream starts) without an explicit close, the sender remains in this map, so hyper never observes EOF and the client hangs while the handle leaks.
Useful? React with 👍 / 👎.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
Devin Review found 2 potential issues.
🐛 1 issue in files not directly in the diff
🐛 Streamed server responses never finish unless the handler explicitly closes them (src/interpreter/mod.rs:8830-8832)
The sender for a streamed server response is stored in a long-lived interpreter table (server_response_streams insert at src/interpreter/mod.rs:8830-8832) that is never cleared when the request handler finishes, so a streamed response whose handler does not explicitly close it stays open forever and its body never completes.
Impact: a client waiting on such a response hangs indefinitely (the chunked body is never terminated), and the table grows by one entry per streamed request, leaking memory over the life of the server.
Why the documented auto-close never happens
The WFL handle object bound by start streaming response ... as out only stores the text id _server_stream (src/interpreter/mod.rs:8834); the actual mpsc::Sender<Vec<u8>> lives in the interpreter-wide server_response_streams map keyed by that id. The transport turns the matching receiver into the response body via unfold, which only ends (yields None) once every sender is dropped.
The map entry is removed in only two places: an explicit close out (src/interpreter/mod.rs:5341) and the write-after-disconnect path (src/interpreter/mod.rs:8887). Nothing ties the sender's lifetime to the handler's scope or main-loop iteration. When a handler ends (normally, via a caught error, or via a break) without calling close out, the sender remains in the map, the receiver never sees None, and the client's body never terminates. Across many requests the map also accumulates dead senders.
This directly contradicts the shipped documentation, which promises The stream is closed automatically when the handler ends on any path. (Docs/04-advanced-features/web-servers.md:502-503) and the design doc's "Close-on-exit" guarantee.
| stop producing (and `close` any upstream you are proxying). The stream is closed | ||
| automatically when the handler ends on any path. |
There was a problem hiding this comment.
🟡 Documentation promises automatic close of streamed responses that the runtime does not perform
The web servers guide states that a streamed server response is closed automatically when the handler ends on any path (Docs/04-advanced-features/web-servers.md:502-503), but the runtime only ends the response when the code explicitly calls close out or the client disconnects, so the prose describes behavior the implementation does not have.
Impact: readers relying on the documented guarantee will ship handlers that omit close, causing responses that never finish for their clients.
Rule basis
CLAUDE.md's "Docs Must Be Honest" policy forbids describing runtime behavior that does not exist. The streamed-response sender is held in the interpreter-wide server_response_streams map with no scope-bound drop (src/interpreter/mod.rs:8830-8832), and is removed only by an explicit close (src/interpreter/mod.rs:5341) or on a write-after-disconnect. There is no code path that closes the stream when a handler ends, so this doc claim is inaccurate.
Prompt for agents
The sentence 'The stream is closed automatically when the handler ends on any path.' at Docs/04-advanced-features/web-servers.md:502-503 does not match the implementation, which only ends a streamed response on an explicit `close out` or a client disconnect. Either implement scope-bound auto-close (see the related runtime finding) or reword this to state that the handler must call `close out` to finalize the response body, so the docs remain honest about current behavior.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Pull request overview
This PR adds end-to-end HTTP streaming support to WFL: client-side streaming response handles with incremental reads, server-side streamed responses (chunk/line writes), and an opt-in main loop concurrently: execution mode so slow handlers don’t block other requests.
Changes:
- Introduces new AST statements + parser/analyzer/typechecker/interpreter support for outbound HTTP response streaming and incremental
next chunk|linepulls. - Adds streamed server responses via
start streaming response+write line|chunk+flush, implemented with a bounded channel for backpressure. - Implements
main loop concurrently:using cooperative single-thread concurrency, and adds tests + docs + validated doc examples.
Reviewed changes
Copilot reviewed 24 out of 24 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/http_stream_test.rs | Adds parser + runtime tests for outbound streaming handles and incremental reads. |
| tests/http_server_streaming_test.rs | Adds parser + runtime tests for streamed server responses (write line/chunk, flush, close). |
| tests/concurrent_main_loop_test.rs | Adds parser + runtime tests verifying concurrent vs serial main loop behavior. |
| TestPrograms/docs_examples/web_servers/streaming_response.wfl | New docs example snippet for streaming server responses. |
| TestPrograms/docs_examples/web_servers/concurrent_main_loop.wfl | New docs example snippet for main loop concurrently:. |
| TestPrograms/docs_examples/interoperability/streaming_response.wfl | New docs example snippet for outbound streaming reads. |
| TestPrograms/docs_examples/_meta/manifest.json | Registers new doc snippets in the validation manifest. |
| src/typechecker/mod.rs | Adds typechecking hooks for new streaming statements and concurrent main loop binding behavior. |
| src/transpiler/javascript.rs | Explicitly rejects streaming HTTP statements in JS transpilation; updates async detection. |
| src/parser/stmt/web.rs | Implements parsing for start streaming response ... as <out> and flush <out>. |
| src/parser/stmt/processes.rs | Extends wait for ... parsing to support `wait for next chunk |
| src/parser/stmt/io.rs | Adds stream response as <name> and `write line |
| src/parser/stmt/control_flow.rs | Parses main loop concurrently: and stores the flag on the AST. |
| src/parser/mod.rs | Dispatches new start ... and flush ... statements at the top-level statement parser. |
| src/parser/ast.rs | Adds new statement variants and the MainLoop.concurrent flag with documentation. |
| src/interpreter/mod.rs | Implements outbound stream handle parking/pulling, server streaming replies, and concurrent main loop execution. |
| src/analyzer/mod.rs | Adds analyzer support for new statements and bound variables. |
| Docs/development/response-streaming-design.md | Adds/updates design/status tracking doc for the streaming feature set. |
| Docs/development/concurrency-phase-plan.md | Updates the concurrency phase tracker to reflect Phase 1 landing and review checkpoint. |
| Docs/04-advanced-features/web-servers.md | Documents concurrent handlers and streamed server responses with examples. |
| Docs/04-advanced-features/interoperability.md | Documents outbound streaming response reads and handle semantics. |
| Dev diary/2026-07-22-server-response-streaming.md | Dev diary entry describing server streaming design and implementation. |
| Dev diary/2026-07-22-outbound-response-streaming.md | Dev diary entry describing outbound streaming design and implementation. |
| Dev diary/2026-07-22-concurrent-request-handlers.md | Dev diary entry describing the concurrent main loop design and containment behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let port = 8241; | ||
| let _server = start_server_thread(server_code(port, true)); |
| fn server_code(port: u16, concurrently: bool) -> String { | ||
| let marker = if concurrently { " concurrently" } else { "" }; | ||
| format!( | ||
| r#" | ||
| listen on port {port} as srv | ||
| main loop{marker}: | ||
| wait for request comes in on srv as req with timeout 20000 | ||
| store p as req["path"] | ||
| check if p is equal to "/slow": | ||
| wait for 500 milliseconds | ||
| respond to req with "slow" | ||
| otherwise: | ||
| respond to req with "fast" | ||
| end check | ||
| end loop | ||
| "# | ||
| ) | ||
| } |
| let slow_resp = slow.await.unwrap(); | ||
| assert_eq!(slow_resp.text().await.unwrap(), "slow"); | ||
| } |
| **Lifecycle & backpressure:** the body channel is bounded, so a slow client | ||
| slows your `write` calls (backpressure) instead of buffering without bound. If | ||
| the client disconnects, hyper drops the response body and your next `write` to | ||
| that stream fails with a catchable error — use `try`/`when` to detect it and | ||
| stop producing (and `close` any upstream you are proxying). The stream is closed | ||
| automatically when the handler ends on any path. |
| The same limits as buffered requests apply: the running total of body bytes is | ||
| held under `web_server_max_response_size`, each read is bounded by the request's | ||
| timeout, and cooperative cancellation interrupts a read waiting on the peer. A | ||
| mid-stream network error surfaces as a catchable error from the `wait for next | ||
| ...` statement, and every stream is closed when the handle is dropped on any | ||
| exit path. |
| // The server survived the caught error and keeps serving. | ||
| let ok = client | ||
| .get(format!("http://127.0.0.1:{port}/ok")) | ||
| .send() | ||
| .await | ||
| .expect("follow-up request failed"); | ||
| assert_eq!(ok.text().await.unwrap(), "ok"); | ||
| } |
| let _ = slow.await; | ||
| } |
| let status_code = match status { | ||
| Some(expr) => { | ||
| let v = self.evaluate_expression(expr, Rc::clone(&env)).await?; | ||
| match &v { | ||
| Value::Number(n) => *n as u16, | ||
| _ => { | ||
| return Err(RuntimeError::new( | ||
| format!("Expected number for status, got {}", v.type_name()), | ||
| *line, | ||
| *column, | ||
| )); | ||
| } | ||
| } | ||
| } | ||
| None => 200, | ||
| }; |
| | Statement::HttpStreamStatement { .. } | ||
| | Statement::WaitForNextChunkStatement { .. } | ||
| | Statement::WaitForNextLineStatement { .. } | ||
| | Statement::StartStreamingResponseStatement { .. } | ||
| | Statement::StreamWriteStatement { .. } | ||
| | Statement::WaitForProcessStatement { .. } |
| if let Some(body) = body { | ||
| let body_type = self.infer_expression_type(body); | ||
| if !matches!( | ||
| body_type, | ||
| Type::Text | ||
| | Type::Number | ||
| | Type::Boolean | ||
| | Type::Unknown | ||
| | Type::Any | ||
| | Type::Error | ||
| ) { | ||
| self.type_error( | ||
| "HTTP request body must be text".to_string(), | ||
| Some(Type::Text), | ||
| Some(body_type), | ||
| *_line, | ||
| *_column, | ||
| ); | ||
| } | ||
| } |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (1)
src/parser/stmt/web.rs (1)
429-464: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winContent-type clause spelling is inconsistent between
respondandstart streaming response.
start streaming response ... and content type "x"(two words) works here because the parser matchesToken::KeywordContentdirectly.respond ... and content_type "x"only works with the underscored single-word form —contentalways lexes as a keyword, soparse_respond_statement's identifier-only branch can never see a two-wordcontent type. Based on learnings, the two-word form was previously confirmed to fail forrespondwithUnexpected token in expression: KeywordAnd. Consider documenting this difference explicitly (or unifying the accepted spelling across both statements) so users don't carry thecontent_type-only assumption fromrespondinto the new streaming statement, or vice versa.🤖 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 `@src/parser/stmt/web.rs` around lines 429 - 464, Unify content-type clause parsing between parse_respond_statement and the start streaming response parser so both accept content type and content_type spellings. Update the respond parser to recognize the Token::KeywordContent followed by the type marker, while preserving existing underscored parsing and expression handling. Ensure both statement forms produce the same content_type result for equivalent input.Source: Learnings
🤖 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 `@Docs/development/concurrency-phase-plan.md`:
- Around line 53-68: Update the Phase 1 concurrency documentation around the
“slow handler does not block other requests” example to state that this applies
when the handler yields at an await point; explicitly note that CPU-bound
handlers can still stall the interpreter thread.
In `@Docs/development/response-streaming-design.md`:
- Line 18: Escape the pipe character in the table cell containing “wait for next
chunk|line” so Markdown treats it as literal content rather than a column
delimiter. Update only that table entry and preserve the existing text and
shipped status.
In `@src/interpreter/mod.rs`:
- Around line 3737-3759: Update execute_concurrent_main_loop and the per-handler
execution path so current_count, in_count_loop, call_stack, and call_depth are
stored in a handler-local execution context rather than shared Interpreter
fields. Create and pass one context per spawned execute_block invocation, ensure
nested calls and count loops read/write that context, and preserve restoration
through normal returns and panic unwinds without sibling state leaking.
In `@src/parser/stmt/io.rs`:
- Around line 796-822: Update the streaming-marker handling near the
`next_token` check so exact `line` or `chunk` identifiers are intercepted only
when the following token is not `Token::KeywordTo`; otherwise preserve the
existing `WriteToStatement` path for variables named `line` or `chunk`. Add
regression coverage for `write line to out` and `write chunk to out` using plain
variables and verify both parse successfully.
In `@src/parser/stmt/processes.rs`:
- Around line 399-432: Restrict the bare-`next` branch in the statement parser
to cases where the following token is `chunk` or `line`; otherwise do not
consume or error on it, and let the existing fallback duration/expression
handling process the identifier. Preserve the dedicated handling for `next
chunk` and `next line` while allowing variables named `next`, including `wait
for next milliseconds`.
In `@src/typechecker/mod.rs`:
- Around line 972-1002: Update the streamed-response branches in the typechecker
to perform the same concrete operand validation as RespondStatement: require a
numeric status, text content type, map headers, and valid stream handles;
enforce Text|Number|Boolean for StreamWriteStatement line payloads and
Text|Binary for chunk payloads. Replace the inference-only checks around
StartStreamingResponseStatement, StreamWriteStatement, and FlushStreamStatement
while preserving existing symbol initialization.
In `@TestPrograms/docs_examples/web_servers/concurrent_main_loop.wfl`:
- Around line 3-5: Update the documentation comment describing concurrent
request handling to state that slow handlers yield to sibling handlers only at
await points, while CPU-bound work still blocks the single interpreter thread;
retain the distinction between concurrent and serial main loops without
overclaiming isolation behavior.
In `@tests/concurrent_main_loop_test.rs`:
- Around line 111-150: In tests/concurrent_main_loop_test.rs lines 111-150, add
a deterministic handler-panic scenario that exercises the concurrent loop’s
catch_unwind path, asserting the panicking request receives HTTP 500 and a
subsequent request still receives a successful response; retain the existing
runtime-error coverage as appropriate. In
Docs/development/concurrency-phase-plan.md lines 57-58, describe only
runtime-error containment as tested unless the new panic-path test is added.
- Around line 79-108: Replace the fixed 80 ms dispatch delay with a test-only
barrier or signal emitted when the /slow handler begins, then await that signal
before sending /fast in tests/concurrent_main_loop_test.rs lines 79-108 and
before timing /fast in lines 155-183. Preserve the existing assertions while
ensuring both tests measure ordering only after /slow has entered its handler.
- Around line 160-182: Update the concurrent request test to assert that the
spawned slow request completes successfully instead of discarding its result at
the final slow.await. Preserve the existing timing and fast-response assertions
while propagating or explicitly failing on both task panics and request errors.
---
Nitpick comments:
In `@src/parser/stmt/web.rs`:
- Around line 429-464: Unify content-type clause parsing between
parse_respond_statement and the start streaming response parser so both accept
content type and content_type spellings. Update the respond parser to recognize
the Token::KeywordContent followed by the type marker, while preserving existing
underscored parsing and expression handling. Ensure both statement forms produce
the same content_type result for equivalent input.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: a9cdec2a-875d-47bd-9466-efabcf094a6e
📒 Files selected for processing (24)
Dev diary/2026-07-22-concurrent-request-handlers.mdDev diary/2026-07-22-outbound-response-streaming.mdDev diary/2026-07-22-server-response-streaming.mdDocs/04-advanced-features/interoperability.mdDocs/04-advanced-features/web-servers.mdDocs/development/concurrency-phase-plan.mdDocs/development/response-streaming-design.mdTestPrograms/docs_examples/_meta/manifest.jsonTestPrograms/docs_examples/interoperability/streaming_response.wflTestPrograms/docs_examples/web_servers/concurrent_main_loop.wflTestPrograms/docs_examples/web_servers/streaming_response.wflsrc/analyzer/mod.rssrc/interpreter/mod.rssrc/parser/ast.rssrc/parser/mod.rssrc/parser/stmt/control_flow.rssrc/parser/stmt/io.rssrc/parser/stmt/processes.rssrc/parser/stmt/web.rssrc/transpiler/javascript.rssrc/typechecker/mod.rstests/concurrent_main_loop_test.rstests/http_server_streaming_test.rstests/http_stream_test.rs
| let _server = start_server_thread(server_code(port, true)); | ||
| tokio::time::sleep(Duration::from_millis(300)).await; | ||
|
|
||
| let client = reqwest::Client::new(); | ||
|
|
||
| // Kick off the slow request first and give it a moment to be dequeued. | ||
| let slow_url = format!("http://127.0.0.1:{port}/slow"); | ||
| let slow = | ||
| tokio::spawn(async move { reqwest::Client::new().get(&slow_url).send().await.unwrap() }); | ||
| tokio::time::sleep(Duration::from_millis(80)).await; | ||
|
|
||
| // The fast request must complete promptly even though /slow is mid-handler. | ||
| let t0 = Instant::now(); | ||
| let fast = client | ||
| .get(format!("http://127.0.0.1:{port}/fast")) | ||
| .send() | ||
| .await | ||
| .expect("fast request failed"); | ||
| let fast_elapsed = t0.elapsed(); | ||
| let fast_body = fast.text().await.unwrap(); | ||
|
|
||
| assert_eq!(fast_body, "fast"); | ||
| assert!( | ||
| fast_elapsed < Duration::from_millis(300), | ||
| "fast request was blocked behind the slow handler ({fast_elapsed:?})" | ||
| ); | ||
|
|
||
| let slow_resp = slow.await.unwrap(); | ||
| assert_eq!(slow_resp.text().await.unwrap(), "slow"); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Replace sleep-based dispatch ordering with a barrier. The 80 ms delay does not guarantee that /slow has been dequeued. A serial implementation can process /fast first and falsely pass the concurrent test or fail the serial test depending on scheduling.
tests/concurrent_main_loop_test.rs#L79-L108: wait for a test-only signal emitted after the slow request enters its handler before sending/fast.tests/concurrent_main_loop_test.rs#L155-L183: use the same signal before timing/fast, so the serial blocking assertion measures the intended ordering.
📍 Affects 1 file
tests/concurrent_main_loop_test.rs#L79-L108(this comment)tests/concurrent_main_loop_test.rs#L155-L183
🤖 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 `@tests/concurrent_main_loop_test.rs` around lines 79 - 108, Replace the fixed
80 ms dispatch delay with a test-only barrier or signal emitted when the /slow
handler begins, then await that signal before sending /fast in
tests/concurrent_main_loop_test.rs lines 79-108 and before timing /fast in lines
155-183. Preserve the existing assertions while ensuring both tests measure
ordering only after /slow has entered its handler.
Deep review: request changesReviewed commit: The happy-path APIs are promising: generic NDJSON-friendly response streaming exists, basic line/chunk reads work, and slow/fast handlers can interleave. However, the lifecycle and concurrency guarantees at the core of this change are not yet satisfied. I do not think this is safe to merge in its current form. Goal assessment
Merge blockers
Additional gaps
Required adversarial test gateBefore merge, I recommend covering:
The parser/API work and generic framing are a solid foundation, but the ownership, cancellation, deadline, and execution-isolation model needs another pass before this can provide the guarantees described in the PR. |
… honesty, test hygiene Automated review (CodeRabbit/Copilot/Devin/Codex) on PR #641 surfaced several issues; this fixes the clear ones: Backward-compatibility regressions (parser): - `write <var> to <file>` with a variable literally named `line`/`chunk` was wrongly intercepted as a stream write and failed to parse. Only treat `line`/`chunk` as the stream marker when it is NOT immediately followed by `to`. Regression test added. - `wait for next <unit>` (e.g. `wait for next milliseconds`) with a variable named `next` was wrongly intercepted as `wait for next chunk|line`. Only take the bare-`next` path when `chunk`/`line` actually follows. Regression test added. Correctness / consistency: - `start streaming response ... with status <n>`: require a whole number in 100..=599 instead of silently wrapping via `as u16`. - `FlushStreamStatement` is now classified async in the transpiler's `stmt_is_async` (it yields), consistent with the other streaming statements. Docs honesty (CLAUDE.md "Docs Must Be Honest"): - Removed the false claim that a streamed server response is "closed automatically when the handler ends"; the handler must `close out` (documented with a finally: recommendation). Client-side wording likewise corrected: a stream is released on EOF/error/explicit close/program exit, not on handle drop. - Concurrent example + phase plan: state the cooperative limitation (a CPU-bound handler with no await still holds the thread), and separate tested runtime-error containment from by-construction catch_unwind panic containment. - Escaped the `chunk|line` pipe in a Markdown table (MD056). Test hygiene (concurrent_main_loop_test): - Moved off ports that collided with another test (8241 -> 8341..8343). - Each server now exposes `/shutdown` (close server + break); tests send it and join the server thread deterministically. - Assert the slow request actually completed instead of discarding its result. Also flags in concurrency-phase-plan.md a known gap for maintainer review: the concurrent loop isolates the environment but not interpreter-level run-state (count/call-stack), which needs a per-handler execution context. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011LxXAm5E4Vy39kRLr8kRnX
| /// Execute a `main loop concurrently:` body. Keeps up to | ||
| /// `CONCURRENT_HANDLER_LIMIT` iterations of `body` in flight at once, each in | ||
| /// its own isolated child scope, driven cooperatively on this single thread | ||
| /// (no threads, no `Send`/`Arc` across the interpreter core). A handler that | ||
| /// errors or panics is contained — its request is resolved with 500 by the | ||
| /// response-completion drop guard — and its siblings keep running. | ||
| async fn execute_concurrent_main_loop( |
The integration runner executes every TestPrograms/*.wfl, but the three new docs-examples need a live upstream / HTTP clients (or run a server loop forever), so they fail/timeout when run standalone. They are validated statically via the docs-examples manifest (layers 1-4). Add the runner's first-line `CI-SKIP:` directive to each so they are skipped by run_integration_tests.sh while still being parse/analyze/lint-validated. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011LxXAm5E4Vy39kRLr8kRnX
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 24 out of 24 changed files in this pull request and generated 8 comments.
Comments suppressed due to low confidence (5)
src/typechecker/mod.rs:940
- This error message/expected-type pair is inconsistent with the allowed types: the check explicitly accepts
NumberandBoolean, but the diagnostic says "must be text" and reportsTextas the expected type. Align the message (and expected type) with the actual accepted set.
self.type_error(
"HTTP request body must be text".to_string(),
Some(Type::Text),
Some(body_type),
*_line,
*_column,
);
}
src/interpreter/mod.rs:3758
execute_concurrent_main_loopruns multipleexecute_blockfutures concurrently against the sameInterpreterinstance. The interpreter has shared mutable run-state (current_count,in_count_loop,call_stack,call_depth, etc.), so concurrent handlers can overwrite each other’s state if they yield at an await inside acountloop or action call. This can produce incorrectcountvalues, wrong error stacks, or mis-enforced call-depth limits.
/// Execute a `main loop concurrently:` body. Keeps up to
/// `CONCURRENT_HANDLER_LIMIT` iterations of `body` in flight at once, each in
/// its own isolated child scope, driven cooperatively on this single thread
/// (no threads, no `Send`/`Arc` across the interpreter core). A handler that
/// errors or panics is contained — its request is resolved with 500 by the
/// response-completion drop guard — and its siblings keep running.
async fn execute_concurrent_main_loop(
&self,
body: &[Statement],
env: &Rc<RefCell<Environment>>,
) -> Result<(Value, ControlFlow), RuntimeError> {
use futures_util::FutureExt;
use futures_util::stream::{FuturesUnordered, StreamExt};
let cap = CONCURRENT_HANDLER_LIMIT.max(1);
let mut futs = FuturesUnordered::new();
let mut last_value = Value::Null;
loop {
self.check_time()?;
// Refill to the concurrency cap. Each iteration gets a fresh isolated
// scope so concurrent requests never clobber each other's variables.
while futs.len() < cap {
let scope = Environment::new_child_env(env);
futs.push(
std::panic::AssertUnwindSafe(self.execute_block(body, scope)).catch_unwind(),
);
tests/concurrent_main_loop_test.rs:97
- Using a fixed TCP port in tests can cause flaky CI failures when the port is already in use on the runner. Prefer binding an ephemeral port (127.0.0.1:0) and using the assigned port for the server/client.
let port = 8341;
tests/concurrent_main_loop_test.rs:135
- Using a fixed TCP port in tests can cause flaky CI failures when the port is already in use on the runner. Prefer binding an ephemeral port (127.0.0.1:0) and using the assigned port for the server/client.
let port = 8342;
tests/concurrent_main_loop_test.rs:183
- Using a fixed TCP port in tests can cause flaky CI failures when the port is already in use on the runner. Prefer binding an ephemeral port (127.0.0.1:0) and using the assigned port for the server/client.
let port = 8343;
| // Internal id used by `wait for next chunk|line` and | ||
| // `close`. Underscore-prefixed to signal "not for | ||
| // direct program use", mirroring request objects. | ||
| stream_map.insert("_stream".to_string(), Value::Text(handle_id.into())); | ||
|
|
||
| let value = Value::Object(Rc::new(RefCell::new(stream_map))); | ||
| match env.borrow_mut().define(variable_name, value) { | ||
| Ok(_) => Ok((Value::Null, ControlFlow::None)), | ||
| Err(msg) => Err(RuntimeError::new(msg, *line, *column)), | ||
| } |
| let mut stream_map = HashMap::new(); | ||
| stream_map.insert("_server_stream".to_string(), Value::Text(handle_id.into())); | ||
| stream_map.insert("status".to_string(), Value::Number(status_code as f64)); | ||
| let value = Value::Object(Rc::new(RefCell::new(stream_map))); | ||
| match env.borrow_mut().define(variable_name, value) { | ||
| Ok(_) => Ok((Value::Null, ControlFlow::None)), | ||
| Err(msg) => Err(RuntimeError::new(msg, *line, *column)), | ||
| } |
| /// line|chunk`. A bounded channel gives backpressure: a slow client slows the | ||
| /// handler's writes. Dropping the sender (handler end, `close`, or a caught | ||
| /// error) closes the body stream and finalizes the response. |
|
|
||
| #[tokio::test] | ||
| async fn test_streamed_response_lines_and_headers() { | ||
| let port = 8231; |
|
|
||
| #[tokio::test] | ||
| async fn test_streamed_response_write_chunk_verbatim() { | ||
| let port = 8232; |
| let body = response.text().await.expect("Failed to read body"); | ||
| assert_eq!(body, "alpha\nbeta\ngamma\n"); | ||
|
|
||
| let _ = server_handle.join(); |
| async fn shutdown(port: u16, server: std::thread::JoinHandle<()>) { | ||
| let _ = reqwest::Client::new() | ||
| .get(format!("http://127.0.0.1:{port}/shutdown")) | ||
| .send() | ||
| .await; | ||
| let _ = tokio::task::spawn_blocking(move || server.join()).await; | ||
| } |
| // write chunk does not append newlines. | ||
| assert_eq!(body, "onetwo"); | ||
|
|
||
| let _ = server_handle.join(); |
Re-review at
|
…file Add the binding Logbie Testing Policy v1.0 as root `testing.md` (required by the policy's §4), together with the WFL project testing profile: supported platform/runtime tuples, one command per test layer + a run-all-presubmit block, critical user/operator journeys, risk triggers (esp. §11.3 concurrency/streaming/ lifecycle for this repo's async work), coverage targets, CI gating, owners, justified N/A layers, and tracked conformance gaps. Wire it into the agent guides: CLAUDE.md and AGENTS.md now list `testing.md` in the governance table and summarize the non-negotiables — Red→Green TDD evidence, risk classification (async/streaming/lifecycle = R3), real-boundary tests with meaningful/negative assertions, no manufactured-green, and PR evidence (§15). Risk class: R0 (documentation/policy; no executable behavior changes). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011LxXAm5E4Vy39kRLr8kRnX
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 27 out of 27 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (4)
src/interpreter/mod.rs:8843
- If binding
variable_namefails, the streaming response has already been started andtxis stored inserver_response_streams, leaving an unreachable open stream (client hangs + leaked sender). Ondefinefailure, ensure the sender is removed/dropped so the body closes and resources are released.
let handle_id = {
let n = self.next_response_stream_id.get();
self.next_response_stream_id.set(n + 1);
format!("respstream{n}")
};
src/interpreter/mod.rs:142
- The doc comment says dropping the streaming-body sender happens on “handler end”, but the sender is stored in
server_response_streamsand will not be dropped when the handler finishes unless WFL code explicitlycloses the stream (or you remove it on error). This is misleading about lifecycle behavior.
/// handler's writes. Dropping the sender (handler end, `close`, or a caught
/// error) closes the body stream and finalizes the response.
src/typechecker/mod.rs:936
- The body type check allows
Text | Number | Boolean, but the error message/expected type currently say “must be text” (and “expected Text”). This is misleading for valid inputs (numbers/bools).
"HTTP request body must be text".to_string(),
Some(Type::Text),
Some(body_type),
src/interpreter/mod.rs:3756
main loop concurrently:runs multipleexecute_block(...)futures concurrently on the sameInterpreter. Several pieces of execution state are stored on the shared interpreter (e.g.current_count/in_count_loop,call_stack,call_depth) and are mutated across.awaitpoints, so concurrent handlers can overwrite each other’s run-state and produce incorrect behavior (wrongcount, corrupted call stacks, etc.).
// Refill to the concurrency cap. Each iteration gets a fresh isolated
// scope so concurrent requests never clobber each other's variables.
while futs.len() < cap {
let scope = Environment::new_child_env(env);
futs.push(
| Statement::StartStreamingResponseStatement { | ||
| request, | ||
| status, | ||
| content_type, | ||
| headers, | ||
| variable_name, | ||
| .. | ||
| } => { | ||
| let _ = self.infer_expression_type(request); | ||
| if let Some(status) = status { | ||
| let _ = self.infer_expression_type(status); | ||
| } | ||
| if let Some(content_type) = content_type { | ||
| let _ = self.infer_expression_type(content_type); | ||
| } | ||
| if let Some(headers) = headers { | ||
| let _ = self.infer_expression_type(headers); | ||
| } | ||
| if !variable_name.is_empty() | ||
| && let Some(symbol) = self.analyzer.get_symbol_mut(variable_name) | ||
| { | ||
| symbol.symbol_type = | ||
| Some(Type::Map(Box::new(Type::Text), Box::new(Type::Unknown))); | ||
| } | ||
| } |
| #[tokio::test] | ||
| async fn test_streamed_response_lines_and_headers() { | ||
| let port = 8231; | ||
| let server_code = format!( | ||
| r#" |
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Docs/development/concurrency-phase-plan.md (1)
49-51: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winSynchronize the stale Phase 1 TODO checklist.
The checklist at Lines 244-256 still marks the parser surface, runtime path, isolation, cap, timeout, and panic handling as unchecked, contradicting this tracker’s “Done” status.
🤖 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 `@Docs/development/concurrency-phase-plan.md` around lines 49 - 51, Update the Phase 1 TODO checklist in the concurrency phase plan so the parser surface, runtime path, isolation, cap, timeout, and panic-handling items match the completed statuses in the Phase 1 tracker. Change only the stale unchecked markers; preserve the existing checklist wording and structure.
🤖 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 `@AGENTS.md`:
- Around line 15-21: Make the testing-policy adoption status consistent across
all governance references: update AGENTS.md lines 15-21 and 125-128, CLAUDE.md
lines 17-23 and 163-167, and testing.md lines 137-143 to state that the policy
is locally adopted and effective, including a consistent effective date;
alternatively, explicitly document why it remains proposed across every site.
Preserve the existing testing requirements while aligning the policy wording,
status, and section titles.
In `@Docs/development/concurrency-phase-plan.md`:
- Around line 59-61: Qualify the transport-layer status mapping in the
concurrency phase plan so it does not claim unconditional 500 containment.
Specifically revise the `ResponseCompletion` drop statement to acknowledge that
handler failures before response startup may instead leave clients waiting until
the default timeout, or fix that failure path and add a regression test covering
it.
- Around line 66-71: Add a dedicated Rust-level regression test at the handler
boundary that injects a panic in one handler, verifies the server remains alive,
and confirms sibling handlers continue operating. Keep the existing
runtime-error test and validate containment through the actual catch_unwind
path, rather than relying only on the panic = "unwind" configuration.
- Around line 56-58: Qualify the concurrent-handler isolation wording to state
that handlers use separate child environments, while parent bindings,
shallow-shared collections, and interpreter-level run state remain shared or
unsafe. Apply this wording in Docs/development/concurrency-phase-plan.md lines
56-58 and replace “own isolated scope” with the same qualified description in
TestPrograms/docs_examples/web_servers/concurrent_main_loop.wfl lines 4-8.
In `@testing.md`:
- Around line 39-49: Expand the “Run all presubmit checks” documentation to
define a complete, reproducible presubmit gate: include documented policy,
secret/dependency, coverage, and risk-triggered checks, plus exact blocking CI
job names and their release mapping. Use one checked-in wrapper/workflow or
explicitly enumerate every required command and platform equivalent, updating
the related CI section as well.
- Around line 94-100: Update the “Coverage & budgets” policy and the related
release-candidate gate section so mandatory coverage and release gates are
either implemented or documented as compliant exceptions. For each remaining
gap, include a ticket ID, owner, deadline, and time-limited Section 17
exception; do not leave “known gap” or target-only language without those
details.
- Around line 15-21: Update the “Supported configuration tuples” table in
testing.md to match the existing CI coverage: remove the macOS row, or document
it only if a corresponding macOS release job or command is added. Preserve the
Linux and Windows entries and their current pipeline status.
---
Outside diff comments:
In `@Docs/development/concurrency-phase-plan.md`:
- Around line 49-51: Update the Phase 1 TODO checklist in the concurrency phase
plan so the parser surface, runtime path, isolation, cap, timeout, and
panic-handling items match the completed statuses in the Phase 1 tracker. Change
only the stale unchecked markers; preserve the existing checklist wording and
structure.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: a20a7843-0e2d-42fb-aafb-38d08562cbf4
📒 Files selected for processing (17)
AGENTS.mdCLAUDE.mdDocs/04-advanced-features/interoperability.mdDocs/04-advanced-features/web-servers.mdDocs/development/concurrency-phase-plan.mdDocs/development/response-streaming-design.mdTestPrograms/docs_examples/interoperability/streaming_response.wflTestPrograms/docs_examples/web_servers/concurrent_main_loop.wflTestPrograms/docs_examples/web_servers/streaming_response.wflsrc/interpreter/mod.rssrc/parser/stmt/io.rssrc/parser/stmt/processes.rssrc/transpiler/javascript.rstesting.mdtests/concurrent_main_loop_test.rstests/http_server_streaming_test.rstests/http_stream_test.rs
🚧 Files skipped from review as they are similar to previous changes (11)
- TestPrograms/docs_examples/web_servers/streaming_response.wfl
- TestPrograms/docs_examples/interoperability/streaming_response.wfl
- Docs/04-advanced-features/interoperability.md
- Docs/04-advanced-features/web-servers.md
- tests/concurrent_main_loop_test.rs
- Docs/development/response-streaming-design.md
- src/transpiler/javascript.rs
- tests/http_stream_test.rs
- src/parser/stmt/io.rs
- tests/http_server_streaming_test.rs
- src/interpreter/mod.rs
| | `testing.md` | **Binding Logbie Testing Policy + WFL testing profile** — Red→Green TDD evidence, required test layers, risk classes, and merge/release gates (see **Testing Guidelines** below) | | ||
|
|
||
| **Agent implications (already in force via governance):** | ||
|
|
||
| - **AI is first-class** — use coding agents freely; same quality bar as hand-written work (tests, docs, compatibility, reviewability). | ||
| - **Backward compatibility is sacred** — never break existing WFL programs without the documented deprecation path. | ||
| - **TDD mandatory** — failing tests first (`tests/`, `TestPrograms/`). | ||
| - **TDD mandatory** — failing tests first (`tests/`, `TestPrograms/`), governed by the binding **Logbie Testing Policy** in root `testing.md`: auditable **Red→Green** evidence for every behavioral change, coverage at the lowest useful layer plus every affected higher layer. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Make the testing-policy adoption status consistent across all governance files.
The agent guides describe testing.md as binding, while its embedded policy remains “Proposed” and effective only “Upon adoption.”
AGENTS.md#L15-L21: state the local adoption status and effective date consistently.AGENTS.md#L125-L128: align the “Binding policy” wording with that status.CLAUDE.md#L17-L23: align the governance reference with the adopted status.CLAUDE.md#L163-L167: align the section title and introduction.testing.md#L137-L143: mark the policy adopted locally, or explicitly document why it remains proposed.
📍 Affects 3 files
AGENTS.md#L15-L21(this comment)AGENTS.md#L125-L128CLAUDE.md#L17-L23CLAUDE.md#L163-L167testing.md#L137-L143
🤖 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 `@AGENTS.md` around lines 15 - 21, Make the testing-policy adoption status
consistent across all governance references: update AGENTS.md lines 15-21 and
125-128, CLAUDE.md lines 17-23 and 163-167, and testing.md lines 137-143 to
state that the policy is locally adopted and effective, including a consistent
effective date; alternatively, explicitly document why it remains proposed
across every site. Preserve the existing testing requirements while aligning the
policy wording, status, and section titles.
| > **Containment:** *runtime-error* containment is tested (an erroring handler | ||
| > does not kill the server). *Panic* containment is by construction via | ||
| > `catch_unwind` on each handler future but is **not** covered by a dedicated | ||
| > test — a deterministic WFL-level Rust panic is not readily expressible from | ||
| > the language, so the guarantee rests on the wrapper + the `panic = "unwind"` | ||
| > gate, not a regression test. |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Add a dedicated panic-containment regression test.
Concurrency and lifecycle changes are R3, so relying only on catch_unwind and panic = "unwind" leaves server survival and sibling containment unverified. Add a Rust-level injected-panic test at the handler boundary.
🤖 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 `@Docs/development/concurrency-phase-plan.md` around lines 66 - 71, Add a
dedicated Rust-level regression test at the handler boundary that injects a
panic in one handler, verifies the server remains alive, and confirms sibling
handlers continue operating. Keep the existing runtime-error test and validate
containment through the actual catch_unwind path, rather than relying only on
the panic = "unwind" configuration.
Source: Coding guidelines
| ### Coverage & budgets | ||
|
|
||
| - Baseline: the policy's defaults (≥80% line / 70% branch overall; ≥90/85 on | ||
| changed code) are the target. **Known gap:** the repo does not yet run an | ||
| automated coverage gate in CI; establishing one is a tracked conformance item | ||
| (§19). Until then, changed behavior MUST still ship behavior/boundary/negative | ||
| tests at the lowest useful layer plus every affected higher layer. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Do not leave mandatory gates as unticketed “known gaps.”
The embedded policy requires coverage thresholds and a release-candidate gate, but the profile downgrades coverage to a target and acknowledges no release-candidate gate without per-gap ticket IDs, owners, deadlines, or a time-limited Section 17 exception. Add the gates or record compliant exceptions before calling the policy adopted.
Also applies to: 125-133
🤖 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 `@testing.md` around lines 94 - 100, Update the “Coverage & budgets” policy and
the related release-candidate gate section so mandatory coverage and release
gates are either implemented or documented as compliant exceptions. For each
remaining gap, include a ticket ID, owner, deadline, and time-limited Section 17
exception; do not leave “known gap” or target-only language without those
details.
…d-collision leak) Address PR #641 review (P1 #8 + Copilot bind-collision leak). `wait for next chunk|line as <name>` and the `open ... stream response as <name>` / `start streaming response as <name>` object binds used `Environment::define`, which errors if the name already exists in scope. Re-reading into the same variable in one scope (or a recycled `main loop` env) therefore failed on the second read — the normal streaming loop pattern. Switch all streaming bindings to `define_or_replace`, matching `wait for request ... as req`. This also removes the bind-collision leak Copilot flagged: `start streaming response` committed the response head + stored the body sender before binding the handle; a binding failure left an unreachable open stream (client hang + leaked sender). With define_or_replace the bind cannot fail after the head is sent. Risk class: R2 (streaming boundary behavior). Red→Green evidence: added `test_next_line_reusing_same_variable_in_one_scope` (observed failing with "Variable 'line' has already been defined" before the fix; passing after) plus `test_next_line_count_loop_reusing_same_variable`. Full http_stream_test suite green (12/12); clippy clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011LxXAm5E4Vy39kRLr8kRnX
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 27 out of 27 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (7)
src/typechecker/mod.rs:937
- The type error message for
HttpStreamStatementbody is inconsistent with the accepted types (Text/Number/Boolean). As written, it claims only text is allowed, and the diagnostic also reportsExpected Text, which can confuse users when the runtime accepts numbers/bools by stringifying them.
self.type_error(
"HTTP request body must be text".to_string(),
Some(Type::Text),
Some(body_type),
*_line,
src/interpreter/mod.rs:3756
main loop concurrently:runs multiple handler executions against the sameInterpreterinstance. The interpreter has per-execution mutable state (e.g.current_count/in_count_loop,call_stack,call_depth) that is mutated and then awaited on inside loops/calls; concurrent handlers can interleave and overwrite each other’s state, producing incorrectcountbehavior and corrupted call-stack/depth tracking.
// Refill to the concurrency cap. Each iteration gets a fresh isolated
// scope so concurrent requests never clobber each other's variables.
while futs.len() < cap {
let scope = Environment::new_child_env(env);
futs.push(
tests/http_server_streaming_test.rs:109
- This test uses a hard-coded port. Rust tests run in parallel by default, so fixed ports can collide with other tests or processes and make CI flaky. Prefer asking the OS for an ephemeral free port.
let port = 8231;
tests/http_server_streaming_test.rs:150
- This test uses a hard-coded port. Rust tests run in parallel by default, so fixed ports can collide with other tests or processes and make CI flaky. Prefer asking the OS for an ephemeral free port.
let port = 8232;
tests/concurrent_main_loop_test.rs:97
- This test uses a hard-coded port. Rust tests run in parallel by default, so fixed ports can collide with other tests or processes and make CI flaky. Prefer asking the OS for an ephemeral free port.
let port = 8341;
tests/concurrent_main_loop_test.rs:135
- This test uses a hard-coded port. Rust tests run in parallel by default, so fixed ports can collide with other tests or processes and make CI flaky. Prefer asking the OS for an ephemeral free port.
let port = 8342;
tests/concurrent_main_loop_test.rs:183
- This test uses a hard-coded port. Rust tests run in parallel by default, so fixed ports can collide with other tests or processes and make CI flaky. Prefer asking the OS for an ephemeral free port.
let port = 8343;
| // `flush <out>` — a bare-identifier target merges into the token. | ||
| Token::Identifier(id) if id == "flush" || id.starts_with("flush ") => { | ||
| self.parse_flush_stream() | ||
| } |
| let value = self.evaluate_expression(source, Rc::clone(env)).await?; | ||
| match &value { | ||
| Value::Object(obj) => match obj.borrow().get("_stream") { | ||
| Some(Value::Text(id)) => Ok(id.to_string()), | ||
| _ => Err(RuntimeError::new( | ||
| "Expected a streaming response handle (from `stream response as ...`), \ | ||
| but this value has no open stream" | ||
| .to_string(), | ||
| line, | ||
| column, | ||
| )), | ||
| }, | ||
| Value::Text(id) => Ok(id.to_string()), | ||
| _ => Err(RuntimeError::new( | ||
| format!( | ||
| "Expected a streaming response handle, got {}", | ||
| value.type_name() | ||
| ), | ||
| line, | ||
| column, | ||
| )), | ||
| } |
| let value = self.evaluate_expression(target, Rc::clone(env)).await?; | ||
| match &value { | ||
| Value::Object(obj) => match obj.borrow().get("_server_stream") { | ||
| Some(Value::Text(id)) => Ok(id.to_string()), | ||
| _ => Err(RuntimeError::new( | ||
| "Expected a server response stream (from `start streaming response as ...`), \ | ||
| but this value has no open stream" | ||
| .to_string(), | ||
| line, | ||
| column, | ||
| )), | ||
| }, | ||
| Value::Text(id) => Ok(id.to_string()), | ||
| _ => Err(RuntimeError::new( | ||
| format!( | ||
| "Expected a server response stream handle, got {}", | ||
| value.type_name() | ||
| ), | ||
| line, | ||
| column, | ||
| )), | ||
| } |
| } else { | ||
| let left = Expression::Variable(rest, marker_line, marker_column); | ||
| match self.cursor.peek().map(|t| &t.token) { | ||
| // `<field> of <object>`, e.g. `write line body of msg to out`. | ||
| Some(Token::KeywordOf) => { | ||
| self.bump_sync(); // Consume "of" | ||
| let object = self.parse_primary_expression()?; | ||
| Expression::FunctionCall { | ||
| function: Box::new(left), | ||
| arguments: vec![crate::parser::ast::Argument { | ||
| name: None, | ||
| value: object, | ||
| }], | ||
| line: marker_line, | ||
| column: marker_column, | ||
| } | ||
| } | ||
| // A bare variable value: the next token starts `to ...`. | ||
| _ => left, | ||
| } |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 83 out of 87 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (3)
TestPrograms/subprocess_comprehensive.wfl:29
- Same portability issue here:
spawn command "cargo --version"requires Cargo to be installed. Use thewfl_binfixture command selected earlier and pass--versionviawith arguments.
TestPrograms/subprocess_comprehensive.wfl:78 - Capturing process output should also avoid assuming
cargoexists. Using the repo’swflbinary with--versionkeeps this test cross-platform and independent of a Rust toolchain.
TestPrograms/subprocess_comprehensive.wfl:98 - These concurrent-process spawns also assume Cargo is installed. Use the
wfl_binfixture command instead so the TestProgram remains runnable anywhere the integration runner’s release binary exists.
| // Test 1: Simple Command Execution | ||
| display "Test 1: Execute Command" | ||
| wait for execute command "echo Hello from subprocess" as cmd_result | ||
| wait for execute command "cargo --version" as cmd_result |
Ensures existing programs using file writes, custom flush actions, and display folds keep their original behavior. This prevents the new web streaming syntax from incorrectly taking over older code patterns during parsing and JavaScript transpilation.
| Statement::StreamWriteStatement { | ||
| target, | ||
| fallback_content: Some(fallback_content), | ||
| .. | ||
| } => { | ||
| // The parser preserves both readings of ambiguous classic syntax | ||
| // such as `write line note to "f.txt"`. JavaScript has no response | ||
| // stream implementation, but it can still emit the pre-streaming | ||
| // file-write reading exactly as it did before. | ||
| let content_expr = self.transpile_expression(fallback_content)?; | ||
| let file_expr = self.transpile_expression(target)?; | ||
| Ok(format!( | ||
| "{}WFL.file.write({}.path, {});\n", | ||
| self.indent(), | ||
| file_expr, | ||
| content_expr | ||
| )) | ||
| } |
| let source = "store line note as \"hello\"\nwrite line note to \"f.txt\""; | ||
| let js = transpile_wfl(source) | ||
| .expect("an ambiguous write with a classic file fallback must transpile"); | ||
| assert_contains(&js, "WFL.file.write(\"f.txt\".path, line_note);"); |
Summary
This change implements the complete streaming response feature set requested downstream: outbound response streaming with incremental reads, streamed server responses, and concurrent request handlers. It ships items 1–4 of the five-capability request and the lifecycle guarantees (item 5) for both client and server streaming.
Key Changes
1. Outbound Response Streaming (Client-Side)
HttpStreamStatementforopen url ... and stream response asWaitForNextChunkStatementandWaitForNextLineStatementto pull body incrementallyIoClientmethods:open_http_stream(): Opens a streaming request, returns status/headers immediately without buffering the bodystream_pull(),next_chunk(),next_line(): Pull body chunks/lines with per-chunk timeout and response-byte ceiling enforcementtake_stream()/put_stream(): Manage live stream handles across async boundariesHttpStreamHandlestruct: Wraps a pinned futures stream, buffered bytes, and read trackingstatus,ok,headers, and internal_streamid to WFL code2. Streamed Server Responses (Server-Side)
StartStreamingResponseStatement:start streaming response to [with status/content type/headers] asStreamWriteStatement:write line|chunk to(frames or raw bytes)FlushStreamStatement:flush(advisory)HandlerReplyenum: ReplacesWflHttpResponseas the per-request oneshot payloadBuffered(WflHttpResponse): Existing fully-buffered responseStreaming { status, content_type, headers, body: mpsc::Receiver<...> }: Head sent immediately, body fed chunk-by-chunk over a bounded channelRESPONSE_STREAM_BUFFER = 64): Provides backpressure so a slow client slows the handler's writes rather than queuing unbounded chunksInterpreterfields:server_response_streams: Map of open response streams by handle idnext_response_stream_id: Counter for handle generationclose, or error) closes the body3. Concurrent Request Handlers
MainLoop.concurrent: bool(defaultfalse)main loop concurrently:setsconcurrent = true; plainmain loopstays serial (byte-compatible)FuturesUnorderedof!Send,&self-borrowing handler futures (catch_unwind-wrapped), nottokio::spawn— up toCONCURRENT_HANDLER_LIMIT = 256. Per-handler run state (count-loop counters, call stack/depth, open response streams and pending requests) is swapped in and out of the shared interpreter perpoll, so concurrent handlers don't clobber each other's execution bookkeeping acrossawaitpoints.awaitpoints — cooperative, I/O-bound concurrency, not parallelism. CPU-bound work with noawaitstill holds the single thread until it yields.storeinside the handler) do not clobber another request's. Top-level (global) bindings and any collections shared through them remain shared by design — mutations to parent globals or shared objects are visible across handlers.4. Lifecycle & Backpressure (Items 5, Client + Server)
stream_pull()close, or teardown) cancels the upstream requesttry/catchclose, or caught error) closes the body and finalizes the response5. Outbound-stream lifecycle P1s (shipped)
Both P1 outbound-stream lifecycle items from the re-review are implemented, each with a real-socket Red→Green regression (mock upstream via
tokio::TcpListener+ the interpreter):run_http_with_budgetcomposes the operation deadline asmin(run/budget remaining, idle, remaining absolute stream deadline)instead of discarding the stream's shorter bound; the absolute clock starts at request initiation (covers connect/header).tests/outbound_stream_deadline_test.rs: a head-then-stall upstream now failswait for next chunkat ~1s underoutbound_stream_max_seconds = 1(was ~10s).RunState.open_http_streams; dropped fromIoClient.stream_handles(cancelling the upstream) on every handler exit —IsolatedHandler::dropfor the concurrent loop and the serial-loop/program cleanup sites.tests/outbound_stream_ownership_test.rs.wait for next line|chunkisselect!ed against the downstream response stream'sSender::closed(), so a browser disconnect cancels a blocked proxy read promptly (dropping the upstream) rather than waiting out the deadline.tests/outbound_stream_disconnect_test.rs.https://claude.ai/code/session_011LxXAm5E4Vy39kRLr8kRnX
``<img src="https://static.devin.ai/assets/gh-open-in-devin-review-light.svg?v=1" alt="Open in Devin Review">``
Summary by CodeRabbit
main loop concurrently:for interleaved request handling (plainmain loop:stays serial).open url ... and stream response as ..., pluswait for next line/chunk ....start streaming response,write line|chunk,flush, and streamingclose.