Add custom response headers support to respond statement - #570
Conversation
…respond` WFL already sends and receives the HTTP QUERY method (RFC 10008) because methods are plain strings end-to-end: clients use `with method "QUERY"` and servers read `req["method"]`. The missing piece for RFC 10008 compliance was server-side response headers — a WFL server had no way to emit `Accept-Query`, `Content-Location`, or `Location`. Add an optional `and headers <map>` clause to the `respond` statement, mirroring the outbound client's `with headers` form (same "headers are a map" concept, nothing to unlearn). The map populates the response's custom headers; the `content_type` clause stays authoritative for Content-Type (a duplicate `Content-Type` key is dropped). - ast: add `headers: Option<Expression>` to `RespondStatement` - parser: parse `and headers <map>` (incl. the merged-identifier form) - interpreter: evaluate the map into `WflHttpResponse.headers` - typechecker: thread the new field through - tests: parser + end-to-end QUERY test proving Accept-Query/Content-Location - docs: web-servers guide gains "Custom Headers" and "QUERY Method" sections - TestPrograms: rfc10008_query_server.wfl demo (CI-SKIP, needs HTTP client) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015YKDtPA3oVRA55g3yUbCQs
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Review limit reached
Next review available in: 41 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThis PR adds an optional ChangesRespond headers and QUERY support
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant WFLServer as WFL Server
participant Interpreter
participant HttpResponse as WflHttpResponse
Client->>WFLServer: QUERY request with body
WFLServer->>Interpreter: dispatch respond statement
Interpreter->>Interpreter: evaluate headers map
Interpreter->>Interpreter: validate header values, skip Content-Type
Interpreter->>HttpResponse: set custom_headers + content_type
HttpResponse-->>Client: response with Accept-Query, Content-Location headers
Possibly related PRs
🚥 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 |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| content, | ||
| status, | ||
| content_type, | ||
| headers: _headers, |
There was a problem hiding this comment.
🟡 Response headers are not validated by the type checker, allowing type errors to reach runtime
The new headers field is explicitly captured but immediately discarded (headers: _headers at src/typechecker/mod.rs:1899) without any type inference or validation, so a wrong-type value (e.g. a number instead of a map) silently passes static analysis and only fails at runtime.
Impact: Users get no compile-time warning when they pass a non-map value to the headers clause, unlike status and content_type which are both type-checked in the same block.
Mechanism: the adjacent status and content_type fields are both validated but headers is skipped
The status field is type-checked at src/typechecker/mod.rs:1919-1933 (must be Number) and content_type is type-checked at src/typechecker/mod.rs:1936-1947 (must be Text). The new headers field should similarly be checked (must be Object/map), but the underscore prefix on _headers suppresses the Rust unused-variable warning and no validation code was added.
Additionally, the semantic analyzer at src/analyzer/mod.rs:1597-1615 and the static analyzer at src/analyzer/static_analyzer.rs:848-863 both use .. to absorb the new field, so the headers expression is never semantically analyzed and variables used in it will be falsely reported as unused.
Prompt for agents
The new `headers` field on RespondStatement is bound as `_headers` in the type checker (src/typechecker/mod.rs:1899) but never validated. Three places need updating:
1. src/typechecker/mod.rs around line 1899: Change `headers: _headers` to `headers`, and after the content_type check block (around line 1947), add a block that calls `self.infer_expression_type` on the headers expression if present. The expected type would be Object (map), though the type system may not have a dedicated Map type — check how other map-typed expressions are validated in this file.
2. src/analyzer/mod.rs around line 1597-1615: The RespondStatement match arm lists request, content, status, content_type with `..` absorbing the rest. Add `headers` to the destructure and call `self.analyze_expression(headers_expr)` if Some, following the same pattern as status and content_type.
3. src/analyzer/static_analyzer.rs around line 848-863: Same pattern — add `headers` to the destructure and call `self.mark_used_in_expression(headers_expr, usages)` if Some, so the headers variable is not falsely reported as unused.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Pull request overview
Adds first-class support for setting custom HTTP response headers from WFL web servers via an optional and headers <map> clause on the respond statement (intended to enable RFC 10008 / HTTP QUERY flows and related headers like Accept-Query and Content-Location).
Changes:
- Extend
respondparsing/AST to capture an optional headers map expression (order-independent withstatus/content_type). - Interpret the headers map at runtime and attach resulting headers to the HTTP response (while preventing
Content-Typeconflicts). - Add docs, tests, and an RFC 10008 demo program illustrating usage.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
src/parser/stmt/web.rs |
Parses optional and headers <map> clause for respond, allowing any clause order. |
src/parser/ast.rs |
Extends RespondStatement AST with headers: Option<Expression>. |
src/interpreter/mod.rs |
Evaluates header map at runtime and populates response headers. |
src/typechecker/mod.rs |
Updates pattern match for the new headers field in RespondStatement. |
Docs/04-advanced-features/web-servers.md |
Documents custom response headers and adds an RFC 10008 QUERY section and examples. |
tests/respond_headers_test.rs |
Adds parser/backward-compat tests and an end-to-end QUERY integration test validating headers. |
TestPrograms/rfc10008_query_server.wfl |
Provides a runnable RFC 10008 QUERY server demo (CI-skipped). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| content, | ||
| status, | ||
| content_type, | ||
| headers: _headers, | ||
| line: _line, |
| if name.eq_ignore_ascii_case("content-type") { | ||
| continue; | ||
| } |
| _ => { | ||
| return Err(RuntimeError::new( | ||
| format!( | ||
| "Response header '{name}' must be text, got {}", |
| ``` | ||
|
|
||
| **Notes:** | ||
| - The map keys are header names and the values are header values (text). |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
src/typechecker/mod.rs (1)
1894-1948: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAdd a type check for the new
headersfield.
headersis destructured as_headersand never inspected, unlikecontent_typein this same match arm andheadersinHttpRequestStatement(lines 545-559 above), both of which validate their expression's inferred type. As written, passing a non-map value torespond ... and headers <expr>is only caught at runtime by the interpreter instead of at type-check time.♻️ Proposed fix
Statement::RespondStatement { request: _request, content, status, content_type, - headers: _headers, + headers, line: _line, column: _column, } => { ... // Check content_type if provided (should be text) if let Some(ct_expr) = content_type { let ct_type = self.infer_expression_type(ct_expr); if ct_type != Type::Text && ct_type != Type::Unknown && ct_type != Type::Error { self.type_error( "Content type must be text".to_string(), Some(Type::Text), Some(ct_type), *_line, *_column, ); } } + + // Check headers if provided (should be a map) + if let Some(headers_expr) = headers { + let headers_type = self.infer_expression_type(headers_expr); + if !matches!( + headers_type, + Type::Map(_, _) | Type::Unknown | Type::Any | Type::Error + ) { + self.type_error( + "Response headers must be a map of header names to values".to_string(), + Some(Type::Map(Box::new(Type::Text), Box::new(Type::Text))), + Some(headers_type), + *_line, + *_column, + ); + } + } }🤖 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/typechecker/mod.rs` around lines 1894 - 1948, The RespondStatement branch in src/typechecker/mod.rs is ignoring the new headers field because it is destructured as _headers, so add a type check for headers in the same way HttpRequestStatement validates its headers expression and this branch validates content_type. Inspect the inferred type of the headers expression in the Statement::RespondStatement match arm, emit a type_error when it is not the expected map/object type, and keep the existing content, status, and content_type checks unchanged.
🧹 Nitpick comments (3)
tests/respond_headers_test.rs (3)
92-103: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winInterpreter errors are silently discarded.
let _ = interpreter.interpret(&ast).await;swallows any runtime error (e.g., a parse/typecheck/runtime failure in the WFL server script). If the server fails to start, the test only surfaces a confusing "Failed to send QUERY request" panic instead of the real cause. Consider logging the error for diagnosability.🔧 Suggested fix
- let _ = interpreter.interpret(&ast).await; + if let Err(e) = interpreter.interpret(&ast).await { + eprintln!("WFL server thread failed: {e:?}"); + }🤖 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/respond_headers_test.rs` around lines 92 - 103, The start_server_thread helper currently discards interpreter failures by assigning the result of Interpreter::interpret to _, which hides the real server startup error. Update the async block in start_server_thread to handle the interpret result explicitly and log or print any error before the thread exits, so failures in Parser or Interpreter are visible when tests fail.
109-189: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing negative-path coverage for the headers clause.
Per the interpreter behavior (src/interpreter/mod.rs), unsupported header value types should produce a runtime error, and a
content-typekey in the custom headers map should be silently dropped in favor of thecontent_typeclause. Neither behavior is exercised by this test suite. Consider adding:
- A test asserting a runtime error when a header value is a non-text/number/bool type (e.g., a list or nested map).
- A test asserting that a
Content-Typekey present in the headers map does not override/duplicate thecontent_typeclause value.🤖 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/respond_headers_test.rs` around lines 109 - 189, Add negative-path coverage around the response headers handling in respond_headers_test.rs. Extend the existing QUERY/header tests or add new ones that exercise the interpreter behavior in src/interpreter/mod.rs: verify that a custom headers map with an unsupported value type (such as a list or nested map) causes a runtime error, and verify that when the headers map contains a Content-Type key, it is dropped or ignored so the respond to req ... content_type clause remains the sole source of the response Content-Type. Use the existing respond to req, query_headers, and test_query_response_sets_custom_headers patterns to keep the new tests easy to locate.
125-128: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winFixed sleep for server-bind sync is flaky under CI load.
Waiting a fixed 300ms for the server to bind is a common source of intermittent CI failures under load. Consider polling the port with retries/backoff instead of a flat sleep.
♻️ Suggested fix: retry-based readiness check
- // Give the server time to bind. - tokio::time::sleep(Duration::from_millis(300)).await; + // Poll until the server is accepting connections, with a bounded timeout. + let mut attempts = 0; + loop { + if std::net::TcpStream::connect(format!("127.0.0.1:{port}")).is_ok() { + break; + } + attempts += 1; + assert!(attempts < 50, "Server did not bind in time"); + tokio::time::sleep(Duration::from_millis(20)).await; + }🤖 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/respond_headers_test.rs` around lines 125 - 128, The fixed 300ms sleep in the server startup test is flaky and should be replaced with a readiness check. Update the setup around start_server_thread in respond_headers_test to poll the server port with retries/backoff until it is accepting connections, then proceed with the test instead of using tokio::time::sleep. Keep the change localized to the test helper/setup code so the bind wait becomes deterministic under CI load.
🤖 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/04-advanced-features/web-servers.md`:
- Around line 220-244: The “With Custom Headers” docs describe `respond to
<request> with <content> and headers <map>` too narrowly by saying header values
are text only. Update the wording in this section so it matches the actual
behavior of `respond to` and `headers` in the parser/interpreter: supported
header value types are accepted and then stringified, while `content_type` still
controls `Content-Type`. Keep the map-key/header-name guidance, but remove any
implication that the input contract is text-only.
- Around line 264-272: The QUERY client example in the web-servers docs is
missing the explicit Content-Type header the prose already mentions. Update the
example around the sending QUERY snippet by inlining the request headers map
instead of relying on request_headers, and include the Content-Type value shown
in the text so the example is directly copy-pasteable; use the QUERY client
example and the open url call as the key anchors.
In `@src/interpreter/mod.rs`:
- Around line 5768-5810: The response header filtering in the interpreter’s
header collection logic only excludes content-type, so conflicting
computed/hop-by-hop headers can still be injected. Update the
custom_headers-building code in the response handling path in
src/interpreter/mod.rs to also reject Content-Length and Transfer-Encoding
(alongside content-type) before inserting into the map, so the downstream warp
response builder remains authoritative for those values.
---
Outside diff comments:
In `@src/typechecker/mod.rs`:
- Around line 1894-1948: The RespondStatement branch in src/typechecker/mod.rs
is ignoring the new headers field because it is destructured as _headers, so add
a type check for headers in the same way HttpRequestStatement validates its
headers expression and this branch validates content_type. Inspect the inferred
type of the headers expression in the Statement::RespondStatement match arm,
emit a type_error when it is not the expected map/object type, and keep the
existing content, status, and content_type checks unchanged.
---
Nitpick comments:
In `@tests/respond_headers_test.rs`:
- Around line 92-103: The start_server_thread helper currently discards
interpreter failures by assigning the result of Interpreter::interpret to _,
which hides the real server startup error. Update the async block in
start_server_thread to handle the interpret result explicitly and log or print
any error before the thread exits, so failures in Parser or Interpreter are
visible when tests fail.
- Around line 109-189: Add negative-path coverage around the response headers
handling in respond_headers_test.rs. Extend the existing QUERY/header tests or
add new ones that exercise the interpreter behavior in src/interpreter/mod.rs:
verify that a custom headers map with an unsupported value type (such as a list
or nested map) causes a runtime error, and verify that when the headers map
contains a Content-Type key, it is dropped or ignored so the respond to req ...
content_type clause remains the sole source of the response Content-Type. Use
the existing respond to req, query_headers, and
test_query_response_sets_custom_headers patterns to keep the new tests easy to
locate.
- Around line 125-128: The fixed 300ms sleep in the server startup test is flaky
and should be replaced with a readiness check. Update the setup around
start_server_thread in respond_headers_test to poll the server port with
retries/backoff until it is accepting connections, then proceed with the test
instead of using tokio::time::sleep. Keep the change localized to the test
helper/setup code so the bind wait becomes deterministic under CI load.
🪄 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: fbf1ac60-91e5-402a-ba5f-c39229f6dbab
📒 Files selected for processing (7)
Docs/04-advanced-features/web-servers.mdTestPrograms/rfc10008_query_server.wflsrc/interpreter/mod.rssrc/parser/ast.rssrc/parser/stmt/web.rssrc/typechecker/mod.rstests/respond_headers_test.rs
…#570 review) Address review feedback on the `respond ... and headers <map>` clause: - typechecker: validate the headers expression is a map (mirrors the outbound `open url` headers check); non-map values are now caught at type-check time - analyzer + static_analyzer: analyze/mark-used the headers expression instead of absorbing it with `..`, so undefined header variables are reported and a map used only in the headers clause is no longer falsely flagged unused - interpreter: also drop Content-Length and Transfer-Encoding from the custom headers map (not just Content-Type), so a script cannot inject duplicate or conflicting pipeline-computed headers (RFC 7230 §3.3.2 / response splitting); correct the value-type error message to name text, numbers, and booleans - docs: note numbers/booleans are stringified; the QUERY client example now inlines the Content-Type request header so it is copy-pasteable - tests: type-check + static-analysis regression tests; an end-to-end test that a Content-Type/Content-Length in the map is dropped (single, correct value); surface interpreter errors from the server thread; poll for bind readiness instead of a fixed sleep Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015YKDtPA3oVRA55g3yUbCQs
Summary
Adds support for custom HTTP response headers in the
respondstatement via an optionaland headers <map>clause. This enables WFL servers to implement RFC 10008 (HTTP QUERY method) by allowing them to set headers likeAccept-QueryandContent-Location.Key Changes
Parser (
src/parser/stmt/web.rs): Extendedrespondstatement parsing to accept an optionaland headers <map>clause that can appear in any order alongsidestatusandcontent_typeclauses.AST (
src/parser/ast.rs): Addedheaders: Option<Expression>field toRespondStatementto capture the optional headers map expression.Interpreter (
src/interpreter/mod.rs): Implemented header evaluation logic that:Content-Typeentries to prevent conflicts with thecontent_typeclauseType Checker (
src/typechecker/mod.rs): Updated to handle the newheadersfield in pattern matching.Documentation (
Docs/04-advanced-features/web-servers.md):Tests (
tests/respond_headers_test.rs): Added comprehensive test suite including:Example Program (
TestPrograms/rfc10008_query_server.wfl): Added complete RFC 10008 QUERY server demo showing practical usage.Implementation Details
The design follows WFL's "no-unlearning" principle by mirroring the outbound client's
with headers <map>syntax — same concept, same syntax, nothing new to learn. TheContent-Typeheader is intentionally filtered from the custom headers map to maintain the authoritative role of thecontent_typeclause, preventing response header conflicts.Header values are coerced to strings, supporting text, numbers, and booleans, with clear error messages for unsupported types.
https://claude.ai/code/session_015YKDtPA3oVRA55g3yUbCQs
Summary by CodeRabbit
New Features
respond tostatements.QUERYmethod, including client and server examples.Documentation
QUERYrequests.Tests
QUERYresponses include the expected headers and content type.