From a1381f7b595e9b243731191b6192be80432291cc Mon Sep 17 00:00:00 2001 From: Vinnie Falco Date: Tue, 22 Sep 2026 07:56:43 -0700 Subject: [PATCH 01/19] Render replayed tool calls in the OpenAI wire shape MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replayed assistant tool calls now reach the model endpoint in the same function-call shape the engine already parses back, so a strict OpenAI-protocol upstream accepts the follow-up turn that carries tool results instead of rejecting it. The projection rendered those calls in a neutral shape the endpoint's schema rejects; aligning it to the inbound contract closes the dialect gap, and the test gateway now enforces that contract so a future regression fails loudly. - `crates/promptforge/lua/src/projection.rs` — replayed calls now render as `{id, type, function: {name, arguments}}`, with `arguments` a JSON-encoded string produced by the infallible `Value::to_string` so a serialization miss cannot silently blank it. - `assert_openai_tool_calls` — a pure validator mirroring the inbound parser; the scripted gateway runs it on every recorded body and answers 400 with a diagnostic naming the offending path, so a shape regression fails the suite instead of reaching a real endpoint. - `replayed_tool_calls_reach_the_mock_gateway_in_the_openai_shape` — a tool round plus its follow-up now completes through the schema-checking gateway, and the test pins the replayed call's id, discriminator, name, and re-decoded arguments on the second request. - `crates/promptforge/lua/src/protocol/request.rs` — the neutral stored record is unchanged; only its documentation and the wire rendering move, so replay and determinism are unaffected. Design: new pure-function @ crates/promptforge-api-runtime/src/execute/tests.rs::assert_openai_tool_calls deps: Value Repairs: OpenAI function-call wire shape for replayed assistant tool_calls @ crates/promptforge/lua/src/projection.rs::wire_message - replayed tool calls rendered as the neutral {id, name, arguments} triple were rejected with 400 by strict OpenAI-protocol endpoints Plan: vibe/2026-09-22-1-replayed-tool-calls.md --- .../src/execute/tests.rs | 84 ++++++++- .../src/execute/tests/models_loop.rs | 51 ++++++ .../promptforge/lua/src/projection-tests.rs | 73 +++++++- crates/promptforge/lua/src/projection.rs | 29 +++- .../promptforge/lua/src/protocol/request.rs | 3 +- .../model-client/src/client/wire.rs | 5 +- vibe/2026-09-22-1-replayed-tool-calls.md | 162 ++++++++++++++++++ vibe/ACTIVE | 1 + 8 files changed, 394 insertions(+), 14 deletions(-) create mode 100644 vibe/2026-09-22-1-replayed-tool-calls.md create mode 100644 vibe/ACTIVE diff --git a/crates/promptforge-api-runtime/src/execute/tests.rs b/crates/promptforge-api-runtime/src/execute/tests.rs index c0aab8c9..115180af 100644 --- a/crates/promptforge-api-runtime/src/execute/tests.rs +++ b/crates/promptforge-api-runtime/src/execute/tests.rs @@ -956,6 +956,83 @@ fn sse_response(body: &Value) -> axum::response::Response { .into_response() } +/// Validates every replayed `messages[].tool_calls[]` entry against the +/// OpenAI function-call schema, mirroring `parse_openai_tool_calls` (the +/// engine's own inbound parser, `pub(crate)` to `model-client` and so +/// unreachable from here). +/// +/// The mock gateway owes the suites a strict endpoint: without this check a +/// neutral-shape replay would pass the mock and fail a real OpenAI or vLLM +/// endpoint, which is exactly the regression this pins. A violation is a +/// diagnostic naming the offending path, never a silent pass. +fn assert_openai_tool_calls(body: &Value) -> std::result::Result<(), String> { + let messages = body + .get("messages") + .and_then(Value::as_array) + .ok_or_else(|| "request body had no messages array".to_owned())?; + for (message_index, message) in messages.iter().enumerate() { + let Some(calls) = message.get("tool_calls").filter(|calls| !calls.is_null()) else { + continue; + }; + let calls = calls.as_array().ok_or_else(|| { + format!("messages[{message_index}].tool_calls was present but not an array") + })?; + for (call_index, call) in calls.iter().enumerate() { + let path = format!("messages[{message_index}].tool_calls[{call_index}]"); + if !call.is_object() { + return Err(format!("{path} was not an object")); + } + match call.get("type") { + Some(Value::String(kind)) if kind == "function" => {} + _ => { + return Err(format!("{path}.type must be the string \"function\"")); + } + } + let id = call + .get("id") + .and_then(Value::as_str) + .ok_or_else(|| format!("{path} had no string id"))?; + if id.trim().is_empty() { + return Err(format!("{path}.id was blank")); + } + let function = call + .get("function") + .ok_or_else(|| format!("{path} had no function"))?; + if !function.is_object() { + return Err(format!("{path}.function was not an object")); + } + let name = function + .get("name") + .and_then(Value::as_str) + .ok_or_else(|| format!("{path}.function had no string name"))?; + if name.trim().is_empty() { + return Err(format!("{path}.function.name was blank")); + } + match function.get("arguments") { + Some(Value::String(raw)) => { + let decoded = serde_json::from_str::(raw).map_err(|error| { + format!("{path}.function.arguments was not valid JSON: {error}") + })?; + if !decoded.is_object() { + return Err(format!( + "{path}.function.arguments did not decode to a JSON object" + )); + } + } + None | Some(Value::Null) => { + return Err(format!("{path}.function.arguments was missing")); + } + Some(_) => { + return Err(format!( + "{path}.function.arguments was not a JSON-encoded string" + )); + } + } + } + } + Ok(()) +} + impl ScriptedGateway { /// Starts a gateway serving `responses` in order (repeating the last). async fn start(responses: Vec) -> ScriptedGateway { @@ -969,7 +1046,12 @@ impl ScriptedGateway { .requests .lock() .expect("scripted gateway request log must not be poisoned") - .push(body); + .push(body.clone()); + // A real OpenAI-protocol endpoint rejects a neutral-shape + // replay with 400; the mock owes the suites the same gate. + if let Err(diagnostic) = assert_openai_tool_calls(&body) { + return (StatusCode::BAD_REQUEST, diagnostic).into_response(); + } let index = n.min(state.responses.len() - 1); match &state.responses[index] { GatewayReply::Json(value) => sse_response(value), diff --git a/crates/promptforge-api-runtime/src/execute/tests/models_loop.rs b/crates/promptforge-api-runtime/src/execute/tests/models_loop.rs index d116c9b3..c1e437e9 100644 --- a/crates/promptforge-api-runtime/src/execute/tests/models_loop.rs +++ b/crates/promptforge-api-runtime/src/execute/tests/models_loop.rs @@ -195,6 +195,57 @@ async fn models_loop_repeats_model_tool_rounds_and_appends_each_exchange() { ); } +#[tokio::test(flavor = "current_thread")] +async fn replayed_tool_calls_reach_the_mock_gateway_in_the_openai_shape() { + // The bug report's failing sequence: one round requests a tool, the + // follow-up round replays the assistant call plus its result. The mock + // gateway validates every inbound body against the OpenAI schema (see + // `assert_openai_tool_calls`), so completing the loop at all proves the + // replay passes a strict endpoint; the assertions below pin the exact + // shape on the replayed request. + let gateway = ScriptedGateway::start(vec![ + resp_tool_call("call_1", "echo", "{\"value\":\"one\"}"), + resp_text("done"), + ]) + .await; + let md = loop_prompt( + "local msgs = messages.new()\n\ + msgs:user('echo once')\n\ + models.loop(msgs)\n\ + return msgs[#msgs].content", + ); + let prompt = parse(&md); + let ctx = loop_context(&prompt, echo_tools()); + let out = TokioDriver::new(&ctx, Some(gateway_client(gateway.addr()))) + .drive() + .await + .expect("the replayed tool-call turn must be accepted"); + assert_eq!(out, "done"); + let bodies = gateway.requests(); + assert_eq!(bodies.len(), 2, "one tool round plus the terminal round"); + let assistant = bodies[1]["messages"] + .as_array() + .expect("a request body must include a messages array") + .iter() + .find(|message| message["role"] == "assistant" && message.get("tool_calls").is_some()) + .expect("the replayed request carries the assistant tool-call turn"); + let call = &assistant["tool_calls"][0]; + assert_eq!(call["id"], "call_1", "the call id replays unchanged"); + assert_eq!(call["type"], "function", "the OpenAI discriminator: {call}"); + assert_eq!( + call["function"]["name"], "echo", + "the name sits under function" + ); + let arguments = call["function"]["arguments"] + .as_str() + .expect("arguments is a JSON-encoded string on the wire"); + assert_eq!( + serde_json::from_str::(arguments).expect("the arguments re-decode"), + json!({ "value": "one" }), + "the arguments string decodes back to the original object" + ); +} + #[tokio::test(flavor = "current_thread")] async fn models_loop_dispatches_local_and_bound_tools() { let gateway = ScriptedGateway::start(vec![ diff --git a/crates/promptforge/lua/src/projection-tests.rs b/crates/promptforge/lua/src/projection-tests.rs index e812fb1b..93c51188 100644 --- a/crates/promptforge/lua/src/projection-tests.rs +++ b/crates/promptforge/lua/src/projection-tests.rs @@ -223,8 +223,8 @@ fn a_complete_tool_exchange_projects_verbatim() { json!([ { "role": "user", "content": "call the tools" }, { "role": "assistant", "content": "working", "tool_calls": [ - { "id": "call_1", "name": "echo", "arguments": {} }, - { "id": "call_2", "name": "search", "arguments": {} }, + { "id": "call_1", "type": "function", "function": { "name": "echo", "arguments": "{}" } }, + { "id": "call_2", "type": "function", "function": { "name": "search", "arguments": "{}" } }, ] }, { "role": "tool", "content": "found", "tool_call_id": "call_2" }, { "role": "tool", "content": "echoed", "tool_call_id": "call_1" }, @@ -234,6 +234,73 @@ fn a_complete_tool_exchange_projects_verbatim() { ); } +#[test] +fn a_two_call_assistant_turn_renders_the_openai_wire_shape() { + // The bug report's captured shape: a replayed assistant turn carrying + // two calls must be the OpenAI function-call shape, the exact inverse + // of `parse_openai_tool_calls`. Arguments are asserted by re-decoding + // the wire string, never by raw string equality, so a future + // `preserve_order` feature cannot make this brittle. + let records = vec![ + user("reproduce the bug"), + assistant_calls( + "checking", + vec![ + ToolCallRecord { + id: "call_a".to_owned(), + name: "read_file".to_owned(), + arguments: json!({ "path": "notes.txt", "limit": 3 }), + }, + ToolCallRecord { + id: "call_b".to_owned(), + name: "search".to_owned(), + arguments: json!({ "query": "tool_calls", "recursive": true }), + }, + ], + ), + tool("call_a", "notes"), + tool("call_b", "found"), + assistant("done"), + ]; + let wire = wire(&records); + let calls = wire[1]["tool_calls"] + .as_array() + .expect("the assistant turn holds a tool_calls array"); + assert_eq!(calls.len(), 2, "both calls replay in one turn"); + let expected = [ + ( + "call_a", + "read_file", + json!({ "path": "notes.txt", "limit": 3 }), + ), + ( + "call_b", + "search", + json!({ "query": "tool_calls", "recursive": true }), + ), + ]; + for (call, (id, name, arguments)) in calls.iter().zip(expected) { + assert_eq!(call["id"], id, "the call id replays unchanged"); + assert_eq!( + call["type"], "function", + "the OpenAI discriminator is required: {call}" + ); + assert_eq!( + call["function"]["name"], name, + "the name moves under function" + ); + let encoded = call["function"]["arguments"] + .as_str() + .expect("arguments is a JSON-encoded string on the wire"); + let decoded: serde_json::Value = + serde_json::from_str(encoded).expect("the encoded arguments re-decode as JSON"); + assert_eq!( + decoded, arguments, + "re-decoding must recover the original object, not merely match a string" + ); + } +} + #[test] fn abnormal_edges_heal_into_a_clean_alternation() { let records = vec![ @@ -272,7 +339,7 @@ fn a_text_fragment_merges_into_a_following_tool_call_turn() { json!([ { "role": "user", "content": "hi" }, { "role": "assistant", "content": "let me check", "tool_calls": [ - { "id": "call_1", "name": "echo", "arguments": {} }, + { "id": "call_1", "type": "function", "function": { "name": "echo", "arguments": "{}" } }, ] }, { "role": "tool", "content": "echoed", "tool_call_id": "call_1" }, { "role": "assistant", "content": "done" }, diff --git a/crates/promptforge/lua/src/projection.rs b/crates/promptforge/lua/src/projection.rs index 78f8df76..269a9a97 100644 --- a/crates/promptforge/lua/src/projection.rs +++ b/crates/promptforge/lua/src/projection.rs @@ -22,9 +22,13 @@ //! record held beyond the contract (a copied credential, say) can never //! reach the provider. //! -//! The output is the provider-neutral wire shape the gateway speaks; the -//! projection is recomputed per dispatch for whichever model the call -//! targets, so a later provider-specific mapping changes this one module. +//! The output is the OpenAI wire shape the gateway speaks: replayed +//! assistant `tool_calls` render as the function-call +//! `{id, type, function: {name, arguments}}` object, the exact inverse of the +//! engine's own inbound parser, so records stay neutral while the wire +//! dogfoods the gateway's ingress contract. The projection is recomputed per +//! dispatch for whichever model the call targets, so any later +//! provider-specific mapping changes this one module. use std::collections::BTreeSet; @@ -262,9 +266,11 @@ fn visible_text(content: &MessageContent) -> &str { } /// Converts one validated, projected record into its wire message: exactly -/// the four contract fields, with each tool call rendered as the -/// provider-neutral `{id, name, arguments}` object. This is the metadata -/// strip - nothing else a record ever held can reach the provider. +/// the four contract fields, with each tool call rendered as the OpenAI +/// function-call `{id, type: "function", function: {name, arguments}}` +/// object, `arguments` a JSON-encoded string. This is the metadata strip - +/// nothing else a record ever held can reach the provider - and the exact +/// inverse of the inbound parser, so a replayed turn re-parses. fn wire_message(record: &MessageRecord) -> Message { let content = match &record.content { MessageContent::Text(text) => Value::String(text.clone()), @@ -293,8 +299,15 @@ fn wire_message(record: &MessageRecord) -> Message { .map(|call| { serde_json::json!({ "id": call.id, - "name": call.name, - "arguments": call.arguments, + "type": "function", + "function": { + "name": call.name, + // `Value::to_string` is infallible; never + // `serde_json::to_string(..).unwrap_or_default()`, + // whose failure mode is a silently empty + // `arguments` string. + "arguments": call.arguments.to_string(), + }, }) }) .collect(), diff --git a/crates/promptforge/lua/src/protocol/request.rs b/crates/promptforge/lua/src/protocol/request.rs index 1bd70ed2..4b548d27 100644 --- a/crates/promptforge/lua/src/protocol/request.rs +++ b/crates/promptforge/lua/src/protocol/request.rs @@ -354,7 +354,8 @@ pub enum MessageContent { /// One normalized tool call an assistant message holds: the /// provider-neutral `{id, name, arguments}` record every later component -/// consumes. +/// consumes. The record stays neutral; the projection's `wire_message` +/// renders it as the OpenAI function-call wire shape at dispatch time. #[derive(Debug, Clone, PartialEq, Eq)] pub struct ToolCallRecord { /// The call identifier tool results correlate against. diff --git a/crates/promptforge/model-client/src/client/wire.rs b/crates/promptforge/model-client/src/client/wire.rs index 0361695b..993d5ef9 100644 --- a/crates/promptforge/model-client/src/client/wire.rs +++ b/crates/promptforge/model-client/src/client/wire.rs @@ -30,7 +30,10 @@ pub struct Message { #[serde(skip_serializing_if = "Option::is_none")] pub(crate) tool_call_id: Option, /// For an `assistant` turn that requested tools, the raw `tool_calls` array - /// as received from the backend, echoed back verbatim. + /// as received from the backend, echoed back verbatim on the live path. The + /// projection path instead re-renders each call from its neutral + /// `ToolCallRecord` into the OpenAI function-call shape, so key order and + /// whitespace can differ from the provider's original. #[serde(skip_serializing_if = "Option::is_none")] pub(crate) tool_calls: Option>, } diff --git a/vibe/2026-09-22-1-replayed-tool-calls.md b/vibe/2026-09-22-1-replayed-tool-calls.md new file mode 100644 index 00000000..7e7030ac --- /dev/null +++ b/vibe/2026-09-22-1-replayed-tool-calls.md @@ -0,0 +1,162 @@ +--- +name: Fix replayed tool_calls wire shape +overview: Make the engine's projection render replayed assistant tool_calls in the OpenAI wire shape (the exact inverse of its own inbound parser), pin the contract with an end-to-end api-runtime test whose mock gateway validates inbound tool_calls against the OpenAI schema, and add a bounded, structured gateway log of the upstream error code so the next 400 doesn't need a logging proxy. +todos: + - id: audit-consumers + content: Audit projected-tool_calls consumers and goldens/fixtures; verify the arguments-always-object invariant at every construction path + status: pending + - id: fix-projection + content: Render OpenAI wire shape in wire_message (projection.rs); update the function doc, the module doc, and the ToolCallRecord/Message docs + status: pending + - id: update-tests + content: Update the two existing projection tests and add a wire-shape pin whose assertion re-decodes the arguments string + status: pending + - id: e2e-openai-shape + content: Add an api-runtime end-to-end test whose ScriptedGateway validates inbound tool_calls against the OpenAI schema + status: pending + - id: gateway-diagnostics + content: Log the bounded, escaped upstream error code/type in OpenAiUpstream::post (not the raw body) + status: pending + - id: verify + content: Run promptforge-lua, promptforge-model-client, promptforge-api-runtime, gateway-protocol, and the gateway app suite; then the papergate or local reproduction + status: pending +isProject: false +--- + +# Fix replayed tool_calls wire shape + + + +## Product Requirements + +A run that replays an earlier assistant turn containing tool calls fails against any OpenAI-protocol upstream: the engine renders each replayed call as `{id, name, arguments}` with `arguments` as a JSON object, and strict endpoints (OpenAI, Azure via OpenRouter, vLLM's OpenAI server) reject the request with 400. The first tool-calling turn succeeds; the turn carrying the tool results fails, so any prompt whose section calls a tool and then continues cannot complete through the harness. Observed at master `b64c1c9d`; the same conversation in OpenAI wire shape returns 200 from the same gateway and model. The fix makes the engine emit the OpenAI shape at the one rendering site and pins the contract with an end-to-end test. + +- Problem and users: replayed assistant `tool_calls` reach OpenAI-protocol endpoints in a provider-neutral shape the endpoint's schema rejects (400); affected users are prompt authors and hosts running tool-calling prompts through `harness-api` against `openai`-protocol upstreams, hosted or self-hosted. +- Goals: replayed tool calls reach an `openai`-protocol endpoint as `{"id", "type": "function", "function": {"name", "arguments": ""}}`; the inverse property (projected output re-parsable by the engine's own inbound normalizer) is enforced at the HTTP boundary; the gateway logs the upstream error code/type so a future 400 does not require a logging proxy. +- Non-goals: no gateway request-path rewriting; no new provider dialects or adapter matrix; no change to the neutral record format or to replay/determinism semantics. +- Success criteria: the api-runtime end-to-end suite passes with a mock gateway that validates inbound `tool_calls` against the OpenAI schema; the papergate reproduction's follow-up turn returns 200 against an OpenAI-protocol endpoint. +- Constraints: the gateway's verbatim-passthrough design (WIRE-001, `crates/gateway/protocol/src/wire.rs`) is preserved; the client-facing error envelope stays body-free (F5); `ToolCallRecord.arguments` is always an object. +- Open questions: None + +## Functional Specification + +The engine validates and projects an author-built message list into wire messages immediately before every model dispatch. The projection's rendering of assistant tool-call records changes from the neutral triple to the OpenAI function-call shape; everything else about the projection (validation rules, healing, metadata stripping) is unchanged. The gateway gains a bounded, structured log line when an upstream returns a non-success status. + +- Actors and workflows: the engine's projection (`wire_message`, `crates/promptforge/lua/src/projection.rs`) renders records per dispatch; transports serialize the request verbatim; the gateway validates minimal shape and forwards verbatim; the upstream validates against the OpenAI schema. +- Inputs and outputs: input is the neutral `ToolCallRecord { id, name, arguments }` (`crates/promptforge/lua/src/protocol/request.rs`, lines 355-367); output is `{"id", "type": "function", "function": {"name", "arguments"}}` with `arguments` a JSON-encoded string decoding to an object. +- States and validation: the inverse property holds - projected tool-call turns are re-parsable by `parse_openai_tool_calls` (`crates/promptforge/model-client/src/normalize.rs`, line 192), which requires `type == "function"`, an object `function`, a nonblank string `function.name`, and string `function.arguments` decoding to an object. +- Errors and recovery: the api-runtime mock gateway answers 400 with a diagnostic body when an inbound request violates the OpenAI tool_calls schema, so a shape regression fails the suite naming the shape rather than surfacing a bare 400. +- Security and privacy behavior: the client-facing gateway envelope stays body-free (F5); the new gateway log carries structured `status`, `code`/`type`, and a bounded, control-escaped `error.message`, never the raw upstream body at warn level, because upstream error bodies can echo prompt content or credentials. +- Acceptance criteria: a two-round tool-calling conversation replays through the mock gateway with every `messages[].tool_calls[]` entry schema-valid; the captured failing request from the bug report, re-rendered by the fixed projection, matches the shape that returned 200. + + + + +## Technical Design + +The gateway is OpenAI-compatible at its edge and OpenAI-canonical internally; it forwards to OpenAI upstreams and translates `OpenAI -> provider` for non-OpenAI upstreams. The engine therefore dogfoods the gateway's OpenAI ingress contract rather than inventing a second wire dialect: its outbound `messages` are OpenAI-shaped, exactly like a third-party OpenAI client's. Records stay neutral internally; only the wire rendering changes. With one universal language on the wire there is no third format for the gateway to translate, which removes the dialect mismatch by construction. + +```mermaid +flowchart LR + Provider -->|OpenAI shape| Norm[Normalizer] + Norm --> Rec[ToolCallRecord] + Rec --> Proj[wire_message] + Proj -->|OpenAI shape| Body[request body] + Body --> GW[Gateway] + GW --> Provider +``` + +- Architecture: the engine's inbound parser (`parse_openai_tool_calls`) already requires the OpenAI shape, and its request builder (`crates/promptforge/model-client/src/client/request.rs`) already emits OpenAI dialect for tool schemas, `tool_choice`, and stream options; the projection's neutral rendering was the lone holdout, a shape the engine's own parser would reject. +- Modules and interfaces: `wire_message` in `crates/promptforge/lua/src/projection.rs` (lines 286-302) renders each call as `{"id": call.id, "type": "function", "function": {"name": call.name, "arguments": call.arguments.to_string()}}`; the gateway diagnostics change sits in `OpenAiUpstream::post` (`crates/gateway/protocol/src/upstream.rs`, lines 273-282), shared by chat, embeddings, rerank, speech, and streaming, so the log applies broadly - intended. +- File and public API changes: `crates/promptforge/lua/src/projection.rs` (rendering, function doc lines 264-267, module doc lines 25-27 which currently claim the output is "the provider-neutral wire shape the gateway speaks"); `crates/promptforge/lua/src/protocol/request.rs` (`ToolCallRecord` doc, lines 355-357: record stays neutral, `wire_message` renders the OpenAI wire shape); `crates/promptforge/model-client/src/client/wire.rs` (`Message.tool_calls` field doc: the live path echoes the backend array verbatim while the projection path re-renders from the neutral record, so key order and whitespace can differ from the provider's original); `crates/promptforge/lua/src/projection-tests.rs`; `crates/promptforge-api-runtime/src/execute/tests.rs`; `crates/gateway/protocol/src/upstream.rs`. No public API signatures change. +- Data, persistence, failure, security, and privacy constraints: `arguments` serialization uses `Value::to_string`, which is infallible - not `serde_json::to_string(..).unwrap_or_default()`, whose failure mode would silently send `arguments: ""`, the same class of silent degradation this fix removes; records and run logs are untouched, so replay/determinism is unaffected; the arguments-always-object invariant must hold at every `ToolCallRecord` construction path (inbound parse, Lua `messages.new` builders, test helpers), because a non-object there would stringify to wire `arguments` that decode to a non-object and be rejected by the engine's own parser. + + + + +## Testing Plan + +Unit tests pin the exact wire shape at the projection; an end-to-end api-runtime test enforces the inverse property at the HTTP boundary, which the unit pin cannot give because `parse_openai_tool_calls` is `pub(crate)` to `model-client` and cannot be called from `lua`. The gateway diagnostics change is covered by the existing `upstream.rs` test harness patterns. Manual verification reruns the original reproduction. + +- Unit: update the two existing projection tests that assert the neutral shape - `a_complete_tool_exchange_projects_verbatim` (`crates/promptforge/lua/src/projection-tests.rs`, line 210) and `a_text_fragment_merges_into_a_following_tool_call_turn` (line 262) - from `{"id", "name", "arguments": {}}` to `{"id", "type": "function", "function": {"name", "arguments": "{}"}}` (the `call()` helper at line 42 builds `arguments: json!({})`); add one focused test pinning the wire JSON for a two-call assistant turn mirroring the bug report's captured request, asserting by re-decoding (`serde_json::from_str::` on the `function.arguments` string equals the original object) rather than raw string equality, so a future `preserve_order` feature does not make the test brittle. +- Integration and end-to-end: add `fn assert_openai_tool_calls(body: &Value) -> Result<(), String>` to `crates/promptforge-api-runtime/src/execute/tests.rs`, mirroring `parse_openai_tool_calls`; have the `ScriptedGateway` completion handler (lines 823-1053, which records every inbound body via `requests()`) run it on each request and answer 400 with a diagnostic body on violation, exactly as a real OpenAI endpoint or vLLM's OpenAI server does; with the validator in place, the existing `models_loop_repeats_model_tool_rounds_and_appends_each_exchange` (`crates/promptforge-api-runtime/src/execute/tests/models_loop.rs`, line 147) already exercises the regression, since it drives two tool rounds and a follow-up turn - the exact failing sequence; add a dedicated test (e.g. `replayed_tool_calls_reach_the_mock_gateway_in_the_openai_shape`) that drives the same loop and asserts on `gateway.requests()[1]["messages"]` that the assistant tool-call entry matches the OpenAI schema. +- Regression, security, and performance: the inverse property is the durable guard - any future projected-shape change the inbound parser rejects fails the api-runtime suite; vLLM validates the same schema, so self-hosted endpoints are covered by the same fix; confirm no golden or fixture anywhere asserts the neutral wire shape (regenerate any that do); confirm the gateway log cannot emit raw upstream bodies at warn. +- Exit criteria: `cargo test -p promptforge-lua -p promptforge-model-client -p promptforge-api-runtime -p gateway-protocol` plus the gateway app integration suite (`-p gateway-app`, `tests/it`) all pass; manual rerun of the papergate reproduction from `crates/papergate/TESTING.md` on branch `papergate-harness-api` (this checkout is `master` with no `crates/papergate`) confirms the follow-up turn returns 200, or a local two-round `models.loop` reproduction via the api-runtime test driver substitutes. + + + + +## Decision Record + +- Decisions: + - Fix in the engine's projection, not the gateway: with the gateway OpenAI-canonical internally, the engine dogfoods the OpenAI ingress contract and the mismatch disappears by construction; the gateway's verbatim passthrough (WIRE-001) is preserved and every OpenAI-protocol consumer (gateway endpoints, the local llama upstream, vLLM direct) is fixed at once. User's words: selected "Engine projection (Recommended) - wire_message renders the exact inverse of parse_openai_tool_calls; one function changes, fixes every OpenAI-protocol consumer, records stay neutral". + - Serialize `arguments` with `Value::to_string`, never `serde_json::to_string(..).unwrap_or_default()`: a silent fallback would send `arguments: ""`, the same class of silent degradation the fix exists to remove, and `Value::to_string` cannot fail. User's words: "Do **not** use `serde_json::to_string(..).unwrap_or_default()`". + - Assert the wire pin by re-decoding the `arguments` string, not raw string equality: a future `preserve_order` feature would reorder the compact string and make a string-equality test brittle. User's plan edit. + - Enforce the contract end-to-end through the `ScriptedGateway` rather than cross-crate unit calls: `parse_openai_tool_calls` is `pub(crate)` to `model-client`, so the inverse property can only be pinned at the HTTP boundary. User's plan edit. + - Log structured upstream `code`/`type` plus a bounded, escaped `error.message`, not the raw body: upstream error bodies can echo prompt content or credentials, and the client envelope must stay body-free (F5). User's words: selected "Yes, include it as a final plan step", then revised the step to structured fields. + - Run the consumer/invariant audit before touching code: the audit carries a stop-and-re-plan gate, and running it first makes the gate real instead of retrospective. User's words: "yes apply step reorder". +- Rejected alternatives: + - Gateway ingress heuristic rewriting of `messages[].tool_calls`: breaks the verbatim-passthrough design (WIRE-001), requires shape sniffing of client payloads, and fixes only the gateway path. Revisit only if the gateway ever adopts a second ingress dialect. + - Logging the raw upstream error body at warn level: violates F5's body-free posture toward logs as well as clients. Revisit only with confirmed redaction-layer coverage of the tracing fields. +- Assumptions, risks, and notes: + - Assumption (gated by the audit): no consumer of projected `tool_calls` depends on the neutral wire shape - expected consumers are `harness-models` transports (serialize verbatim), the gateway (presence-only validation), and the engine test client. If any consumer depends on the neutral shape, stop and re-plan. + - Risk: a `ToolCallRecord` construction path (inbound parse, Lua `messages.new` builders, test helpers) could place a non-object in `arguments`; the audit cites the guaranteeing line range for each path. + - Note: this checkout is `master` with no `crates/papergate`; the manual reproduction runs from branch `papergate-harness-api` or is substituted by a local two-round `models.loop` driver. + - Note: `OpenAiUpstream::post` is shared by chat, embeddings, rerank, speech, and streaming, so the new log line applies to all of them; this is intended. + - Note: whether the `gateway/logging` redaction layer (`crates/gateway/logging/src/redact.rs`) applies to the new tracing fields is confirmed during implementation; if it does not, the log keeps to structured code/type plus the escaped `error.message`. + +### Deferred and Out of Scope + +- Deferred: a provider-specific adapter matrix for non-OpenAI upstreams; revisit when a non-OpenAI upstream protocol is added behind the gateway's `Upstream` trait. +- Out of scope: changing the neutral `ToolCallRecord` storage format; changing gateway request-path forwarding; harness-side surfacing of upstream 400 bodies beyond the gateway log line. + + + + +## Project Survey + +- Status: complete +- Build command: `cargo build` (builds only the gateway, the workspace default-member; use `cargo build -p ` for others, e.g. `cargo build -p workshop` for the desktop app) +- Focused test command pattern: `cargo nextest run --locked -p ` +- Component test command pattern: `cargo nextest run --locked -p --all-features` (workshop crates: `cargo nextest run --locked -p workshop -p workshop-server -p workshop-server-api`); structural/boundary harness: `cargo test -p build-xtask` +- Full-suite test command: `cargo nextest run --locked --workspace --exclude workshop --exclude workshop-server --exclude workshop-server-api --all-features`, then doctests via `cargo test --workspace --exclude workshop --exclude workshop-server --exclude workshop-server-api --all-features --doc`; workshop crates separately via `cargo nextest run --locked -p workshop -p workshop-server -p workshop-server-api` +- Linter command: `cargo clippy --workspace --exclude workshop --exclude workshop-server --exclude workshop-server-api --all-targets --all-features -- -D warnings` (workshop: `cargo clippy -p workshop -p workshop-server -p workshop-server-api --all-targets -- -D warnings`); never run standalone `cargo check --workspace` beside clippy except the headless gate `cargo check -p gateway --no-default-features` +- Formatter check command: `cargo fmt --all --check` +- Docs command: `cargo doc --workspace --no-deps --all-features --exclude workshop --exclude workshop-server --exclude workshop-server-api` with `RUSTDOCFLAGS="-D warnings"`; user guide via `mdbook build guide` +- Test placement and naming conventions: unit tests live beside the source file as `foo-tests.rs` siblings wired with `#[path = "foo-tests.rs"] mod tests;` (e.g. `projection.rs` / `projection-tests.rs` in `crates/promptforge/lua/src`), or inline `mod tests`; larger suites get a `tests/` subdirectory inside `src` (e.g. `crates/promptforge/lua/src/protocol/tests/`); integration tests follow Cargo target conventions in crate-level `tests/` trees (e.g. `crates/gateway/app/tests/it/`); benches live in crate-level `benches/` (criterion); nextest profiles and a `heavy` test-group for FFI-heavy STT suites are configured in `.config/nextest.toml` +- Directory map: `crates/` holds the whole workspace: public root crates (`promptforge-api-runtime`, `promptforge-api-types`, `gateway-api-types`, `gateway-api-discovery`, `harness-api`), shared substrate crates (`shared-*`, `shared-vfs`, `workspace-hack`), four manifestless family containers (`crates/promptforge/`, `crates/gateway/`, `crates/workshop/`, `crates/harness/`) holding private family crates (gateway's STT subsystem nests at `crates/gateway/stt/`), and `build-*` meta tooling (`build-xtask` structural harness, `build-ui`, `build-user-guide`, etc.); `crates/shared-ui` is a TypeScript+CSS package, not a Rust crate; `guide/` is the mdbook user guide; `prompts/` holds prompt pipelines; `tools/` holds Node helper scripts; `vibe/` holds planning docs including `archdoc.md`; `.config/` holds nextest and hakari config; `.github/workflows/` holds CI +- Component boundaries (from `vibe/archdoc.md`): executor (sans-I/O deterministic state machine, no host trait objects) <- harness (only production host; tokio runtime, performers, sessions, Turso run log; public surface `harness-api`); gateway (independent server owning model routing and inference lifecycle; public pair `gateway-api-types` + `gateway-api-discovery`); CLI (thin shell adapter); workshop UI (Tauri desktop shell driving `harness-api`, attaching over the gateway protocol); store over the VFS layer (`shared-vfs` backends, `promptforge-vfs` policy gate); Lua VM boundary (sandbox/coroutine bridge, no I/O); shared substrate (`shared-*`, `shared-error-source`) depends on nothing; dependency rules: promptforge-* never depend on gateway/workshop/harness crates, gateway crates never depend on promptforge/workshop crates, workshop crates name only the gateway public pair plus `harness-api`, outside crates enter each family only through its public root crate +- Conventions summary: Rust edition 2024 on the stable toolchain (`rust-toolchain.toml`); workspace lints forbid `unsafe_code`, deny clippy `all`/`pedantic`/`unwrap_used`/`expect_used`, and deny broken rustdoc links; flat source directories by default - one or two related files stay as `foo-bar.rs` kebab siblings with explicit `#[path]` attributes, three or more rehydrate into a `foo/` subdirectory; every workshop-* and harness-* lib.rs opens with a mandatory `## Invariants` doc marker and no file in a marked crate exceeds 500 lines (enforced by `cargo test -p build-xtask`, which also enforces the tier graph and family privacy matrix); behavior changes ship with tests in the same change; comments explain non-obvious constraints and cite upstream issue URLs for workarounds; error messages are written for model consumption (concise, factual, required-vs-actual); CSS/TypeScript for the SPA lives self-contained per feature directory with `--ws-*` design tokens and no `localStorage` + + + + +## Execution Instructions + + + +### Step 1: Render replayed tool_calls in the OpenAI wire shape [completed] + +- Component: `none` +- Audit gate (runs first, per the Decision Record): audit consumers of projected `tool_calls` (expected: `harness-models` transports, the gateway, the engine test client) and all goldens/fixtures for neutral-shape dependencies; verify the arguments-always-object invariant at every `ToolCallRecord` construction path (inbound parse in `crates/promptforge/model-client/src/normalize.rs`, Lua `messages.new` builders, test helpers), citing the guaranteeing line range for each. If any consumer depends on the neutral wire shape, stop and re-plan. +- Change `wire_message` in `crates/promptforge/lua/src/projection.rs` (lines 286-302) to render each call as `{"id", "type": "function", "function": {"name", "arguments"}}` with `arguments` serialized via `Value::to_string` (infallible; never `serde_json::to_string(..).unwrap_or_default()`). +- Update the four doc sites that assert the old contract: the `wire_message` function doc (`projection.rs` lines 264-267), the module doc (lines 25-27), the `ToolCallRecord` doc (`crates/promptforge/lua/src/protocol/request.rs` lines 355-357), and the `Message.tool_calls` field doc (`crates/promptforge/model-client/src/client/wire.rs`). +- Update the two neutral-shape projection tests in `crates/promptforge/lua/src/projection-tests.rs` (`a_complete_tool_exchange_projects_verbatim` line 210, `a_text_fragment_merges_into_a_following_tool_call_turn` line 262) to the OpenAI shape, and add one focused wire-shape pin for a two-call assistant turn that asserts by re-decoding the `function.arguments` string (`serde_json::from_str::` equals the original object), not raw string equality. +- Add `fn assert_openai_tool_calls(body: &Value) -> Result<(), String>` to `crates/promptforge-api-runtime/src/execute/tests.rs`, mirroring `parse_openai_tool_calls`; run it in the `ScriptedGateway` completion handler (lines 823-1053) on every recorded request, answering 400 with a diagnostic body on violation; add the dedicated test `replayed_tool_calls_reach_the_mock_gateway_in_the_openai_shape` asserting on `gateway.requests()[1]["messages"]`. The existing `models_loop_repeats_model_tool_rounds_and_appends_each_exchange` (`execute/tests/models_loop.rs` line 147) then exercises the regression. +- Tests: `cargo test -p promptforge-lua -p promptforge-api-runtime`; confirm no golden or fixture anywhere asserts the neutral wire shape (regenerate any that do). +- One commit containing the rendering change, doc updates, and all tests above. + + + + + +### Step 2: Log bounded upstream error diagnostics and run exit criteria + +- Component: `none` +- Add a structured, bounded log line in `OpenAiUpstream::post` (`crates/gateway/protocol/src/upstream.rs`, lines 273-282) when an upstream returns a non-success status: warn for 5xx, debug for 4xx, with fields `status`, `code`/`type`, and a bounded, control-escaped `error.message`; never log the raw upstream body (F5). Confirm whether the `gateway/logging` redaction layer (`crates/gateway/logging/src/redact.rs`) applies to these tracing fields; if it does not, keep to structured code/type plus the escaped message. Note this post is shared by chat, embeddings, rerank, speech, and streaming - the broad application is intended. Cover with the existing `upstream.rs` test harness patterns. +- Run the exit criteria: `cargo test -p promptforge-lua -p promptforge-model-client -p promptforge-api-runtime -p gateway-protocol` plus the gateway app integration suite (`-p gateway-app`, `tests/it`); then rerun the papergate reproduction from `crates/papergate/TESTING.md` on branch `papergate-harness-api` (this checkout is `master` with no `crates/papergate`) confirming the follow-up turn returns 200, or substitute a local two-round `models.loop` reproduction via the api-runtime test driver. +- One commit containing the log line and its tests; verification runs after it, per the Testing Plan. + + + + diff --git a/vibe/ACTIVE b/vibe/ACTIVE new file mode 100644 index 00000000..5c2f5b18 --- /dev/null +++ b/vibe/ACTIVE @@ -0,0 +1 @@ +vibe/2026-09-22-1-replayed-tool-calls.md From 83c638c91dc61feccaa40299ce99a6b17ae4a107 Mon Sep 17 00:00:00 2001 From: Vinnie Falco Date: Tue, 22 Sep 2026 08:37:43 -0700 Subject: [PATCH 02/19] =?UTF-8?q?=EF=BB=BFLog=20bounded=20upstream=20error?= =?UTF-8?q?=20diagnostics?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A non-success upstream response now emits one structured log line carrying the status and the upstream error code, type, and message, at warn level for a server error and debug level for a client error. The raw response body never reaches the log, because an upstream error can echo prompt content or credentials. Each diagnostic field is bounded and control-escaped, so a crafted message cannot forge log lines, inject terminal controls, or flood the log, and a body outside the expected error envelope yields no diagnostics at all. - `UpstreamErrorDiagnostics` decodes only the upstream error envelope in `from_body`, yielding empty fields for any other shape so the raw body has no path into the log even as a fallback. - `bounded_error_field` renders a missing or non-string field as the empty string rather than a fallback that could leak the body. - `escape_control` escapes newlines, carriage returns, tabs, and other control characters, keeping at most `MAX_ERROR_MESSAGE_CHARS` input characters before escaping can expand them. - `capture_logs` takes the subscriber level and `capture_warnings` delegates to it, so the new tests assert both the warn and the debug paths. - `crates/gateway/protocol/src/upstream.rs` logs one structured event per failed response, at warn for a server error and debug for a client error, with `status`, `code`, `type`, and the escaped `error.message`. Design: new encapsulated-invariant @ crates/gateway/protocol/src/upstream.rs::UpstreamErrorDiagnostics Design: new pure-function @ crates/gateway/protocol/src/upstream.rs::bounded_error_field deps: Option<&serde_json::Value> Design: new pure-function @ crates/gateway/protocol/src/upstream.rs::escape_control deps: &str, usize Plan: vibe/2026-09-22-1-replayed-tool-calls.md --- crates/gateway/protocol/src/upstream.rs | 251 ++++++++++++++++++++++- vibe/2026-09-22-1-replayed-tool-calls.md | 2 +- 2 files changed, 247 insertions(+), 6 deletions(-) diff --git a/crates/gateway/protocol/src/upstream.rs b/crates/gateway/protocol/src/upstream.rs index 019a059a..b9d5e7be 100644 --- a/crates/gateway/protocol/src/upstream.rs +++ b/crates/gateway/protocol/src/upstream.rs @@ -5,6 +5,7 @@ //! unchanged. Adding an Anthropic or pack upstream later is a new implementation //! behind this same trait, with no change to routing or the request handler. +use std::fmt::Write as _; use std::time::Duration; use async_trait::async_trait; @@ -275,6 +276,29 @@ impl OpenAiUpstream { let body = crate::http_util::read_body_capped(response, crate::http_util::MAX_ERROR_BODY) .await; + // F5: the raw body may echo prompt content or credentials, so only + // its OpenAI error envelope reaches the log, bounded and escaped. + let diagnostics = UpstreamErrorDiagnostics::from_body(&body); + let code = diagnostics.code.as_str(); + let kind = diagnostics.kind.as_str(); + let message = diagnostics.message.as_str(); + if status.is_server_error() { + tracing::warn!( + status = status.as_u16(), + code = %code, + r#type = %kind, + error.message = %message, + "upstream returned a server error" + ); + } else { + tracing::debug!( + status = status.as_u16(), + code = %code, + r#type = %kind, + error.message = %message, + "upstream returned a client error" + ); + } let body: String = body.chars().take(2000).collect(); return Err(ProtocolError::UpstreamStatus { status: status.as_u16(), @@ -308,6 +332,73 @@ impl OpenAiUpstream { } } +/// Maximum characters retained from an upstream error field for diagnostics. +/// Applied per field on the decoded string, before control escaping can expand +/// it, so one event cannot flood the log with an oversized upstream message. +const MAX_ERROR_MESSAGE_CHARS: usize = 512; + +/// The bounded, safe diagnostics extracted from a non-success upstream body. +/// +/// The raw body is never retained or logged: only the OpenAI error envelope's +/// `code`, `type`, and `message` are read, and each string is control-escaped +/// and bounded. A body outside that shape (or a non-string field) yields an +/// empty value rather than a fallback that could leak the body. +#[derive(Debug, Default)] +struct UpstreamErrorDiagnostics { + code: String, + kind: String, + message: String, +} + +impl UpstreamErrorDiagnostics { + fn from_body(body: &str) -> Self { + let Ok(value) = serde_json::from_str::(body) else { + return Self::default(); + }; + match value.get("error") { + Some(serde_json::Value::Object(error)) => Self { + code: bounded_error_field(error.get("code")), + kind: bounded_error_field(error.get("type")), + message: bounded_error_field(error.get("message")), + }, + Some(serde_json::Value::String(message)) => Self { + message: escape_control(message, MAX_ERROR_MESSAGE_CHARS), + ..Self::default() + }, + _ => Self::default(), + } + } +} + +/// Renders a string-valued error field, control-escaped and bounded; a +/// missing or non-string field renders as the empty string. +fn bounded_error_field(value: Option<&serde_json::Value>) -> String { + value + .and_then(serde_json::Value::as_str) + .map_or_else(String::new, |text| { + escape_control(text, MAX_ERROR_MESSAGE_CHARS) + }) +} + +/// Escapes every control character so a crafted upstream message cannot forge +/// log lines or inject terminal control sequences, keeping at most `max_chars` +/// input characters. +fn escape_control(text: &str, max_chars: usize) -> String { + let mut escaped = String::new(); + for character in text.chars().take(max_chars) { + match character { + '\n' => escaped.push_str("\\n"), + '\r' => escaped.push_str("\\r"), + '\t' => escaped.push_str("\\t"), + control if control.is_control() => { + let _ = write!(escaped, "\\u{{{:x}}}", control as u32); + } + other => escaped.push(other), + } + } + escaped +} + /// Parses an upstream SSE byte stream into validated [`ChatChunk`]s. /// /// Each `data:` line holds one JSON chunk; blank lines, comments, and the @@ -846,20 +937,26 @@ mod tests { } } - /// Installs a WARN-level subscriber writing to a fresh capture buffer for - /// the current thread (tokio's current-thread test runtime keeps every - /// poll on this thread, so the parser's warnings land in the buffer). - fn capture_warnings() -> (LogBuffer, tracing::subscriber::DefaultGuard) { + /// Installs a subscriber at `max_level` writing to a fresh capture buffer + /// for the current thread (tokio's current-thread test runtime keeps every + /// poll on this thread, so the events land in the buffer). + fn capture_logs(max_level: tracing::Level) -> (LogBuffer, tracing::subscriber::DefaultGuard) { let buffer = LogBuffer::default(); let subscriber = tracing_subscriber::fmt() .with_writer(buffer.clone()) .with_ansi(false) - .with_max_level(tracing::Level::WARN) + .with_max_level(max_level) .finish(); let guard = tracing::subscriber::set_default(subscriber); (buffer, guard) } + /// Installs a WARN-level subscriber writing to a fresh capture buffer for + /// the current thread, for the parser's warning assertions. + fn capture_warnings() -> (LogBuffer, tracing::subscriber::DefaultGuard) { + capture_logs(tracing::Level::WARN) + } + #[tokio::test] async fn stream_malformed_chunks_are_logged_and_skipped() { // An undecodable or shape-invalid chunk is logged and skipped; the @@ -1046,6 +1143,150 @@ mod tests { let _ = handle.join(); } + #[tokio::test] + async fn server_error_logs_structured_diagnostics_without_the_raw_body() { + // F5: a 5xx logs at WARN with the structured status/code/type and the + // escaped error.message, but never the raw body's other content - an + // upstream error body can echo prompt content or credentials. + let (logs, _guard) = capture_logs(tracing::Level::DEBUG); + let body = r#"{"error":{"message":"line1\nline2","type":"server_error","code":"internal"},"echo":{"prompt":"BODY_ONLY_SENTINEL"}}"#; + let (base, handle) = serve_once("500 Internal Server Error", body); + let upstream = OpenAiUpstream::new(&base, Secret::new(String::new())); + let _ = upstream + .send(request("m"), "u") + .await + .expect_err("5xx fails"); + let _ = handle.join(); + let logs = logs.contents(); + assert!( + logs.lines() + .any(|line| line.contains("WARN") + && line.contains("upstream returned a server error")), + "a 5xx logs at warn: {logs}" + ); + assert!(logs.contains("status=500"), "status is structured: {logs}"); + assert!(logs.contains("code=internal"), "code is structured: {logs}"); + assert!( + logs.contains("type=server_error"), + "type is structured: {logs}" + ); + assert!( + logs.contains(r"line1\nline2"), + "the error message is control-escaped: {logs}" + ); + assert!( + !logs.contains("BODY_ONLY_SENTINEL"), + "unrelated body content is never logged: {logs}" + ); + } + + #[tokio::test] + async fn client_error_logs_at_debug_not_warn() { + // A 4xx is the caller's error, not the backend's: it logs at debug so + // the warn stream stays reserved for server-side failures. + let (logs, _guard) = capture_logs(tracing::Level::DEBUG); + let body = r#"{"error":{"message":"unknown model","type":"invalid_request_error","code":"model_not_found"}}"#; + let (base, handle) = serve_once("400 Bad Request", body); + let upstream = OpenAiUpstream::new(&base, Secret::new(String::new())); + let _ = upstream + .send(request("m"), "u") + .await + .expect_err("4xx fails"); + let _ = handle.join(); + let logs = logs.contents(); + assert!( + logs.lines() + .any(|line| line.contains("DEBUG") + && line.contains("upstream returned a client error")), + "a 4xx logs at debug: {logs}" + ); + assert!( + !logs + .lines() + .any(|line| line.contains("WARN") + && line.contains("upstream returned a client error")), + "a 4xx never logs at warn: {logs}" + ); + } + + #[tokio::test] + async fn non_json_error_body_is_never_logged_raw() { + // A body outside the OpenAI error shape yields no diagnostic fields; + // the raw body must not be logged as a fallback. + let (logs, _guard) = capture_logs(tracing::Level::DEBUG); + let (base, handle) = serve_once("502 Bad Gateway", "RAW_BODY_SECRET"); + let upstream = OpenAiUpstream::new(&base, Secret::new(String::new())); + let _ = upstream + .send(request("m"), "u") + .await + .expect_err("5xx fails"); + let _ = handle.join(); + let logs = logs.contents(); + assert!(logs.contains("status=502"), "the status still logs: {logs}"); + assert!( + !logs.contains("RAW_BODY_SECRET"), + "an unparseable body is never logged raw: {logs}" + ); + } + + #[tokio::test] + async fn control_characters_in_the_error_message_are_escaped() { + // A crafted message cannot forge log lines or inject terminal control: + // newlines, carriage returns, tabs, and escape bytes are escaped. + let (logs, _guard) = capture_logs(tracing::Level::WARN); + let body = r#"{"error":{"message":"forged\r\nwarning: fake\t\u001b[31mred"}}"#; + let (base, handle) = serve_once("500 Internal Server Error", body); + let upstream = OpenAiUpstream::new(&base, Secret::new(String::new())); + let _ = upstream + .send(request("m"), "u") + .await + .expect_err("5xx fails"); + let _ = handle.join(); + let logs = logs.contents(); + assert!( + logs.contains(r"forged\r\nwarning: fake\t"), + "control characters are escaped: {logs}" + ); + assert!( + logs.contains(r"\u{1b}[31mred"), + "an ANSI escape is escaped: {logs}" + ); + assert!( + !logs.contains("forged\r\nwarning"), + "no raw CRLF survives in the message: {logs}" + ); + assert!( + !logs.contains("warning: fake\t"), + "no raw tab survives in the message: {logs}" + ); + } + + #[tokio::test] + async fn an_over_long_error_message_is_bounded() { + // The message is bounded independently of the body cap so one event + // cannot flood the log with a huge upstream message. + let (logs, _guard) = capture_logs(tracing::Level::WARN); + let message = "z".repeat(MAX_ERROR_MESSAGE_CHARS + 200); + let body = format!("{{\"error\":{{\"message\":\"{message}\"}}}}"); + let (base, handle) = serve_once("500 Internal Server Error", &body); + let upstream = OpenAiUpstream::new(&base, Secret::new(String::new())); + let _ = upstream + .send(request("m"), "u") + .await + .expect_err("5xx fails"); + let _ = handle.join(); + let logs = logs.contents(); + assert!( + logs.contains(&"z".repeat(MAX_ERROR_MESSAGE_CHARS)), + "the bound keeps a full-length prefix" + ); + assert!( + !logs.contains(&"z".repeat(MAX_ERROR_MESSAGE_CHARS + 1)), + "the message never exceeds the bound: {}", + logs.len() + ); + } + /// A server that accepts the connection and then never sends a response, so /// the client's request deadline (not an idle read) is what must fire. fn serve_stalled() -> (String, JoinHandle<()>) { diff --git a/vibe/2026-09-22-1-replayed-tool-calls.md b/vibe/2026-09-22-1-replayed-tool-calls.md index 7e7030ac..25f11b33 100644 --- a/vibe/2026-09-22-1-replayed-tool-calls.md +++ b/vibe/2026-09-22-1-replayed-tool-calls.md @@ -150,7 +150,7 @@ Unit tests pin the exact wire shape at the projection; an end-to-end api-runtime -### Step 2: Log bounded upstream error diagnostics and run exit criteria +### Step 2: Log bounded upstream error diagnostics and run exit criteria [completed] - Component: `none` - Add a structured, bounded log line in `OpenAiUpstream::post` (`crates/gateway/protocol/src/upstream.rs`, lines 273-282) when an upstream returns a non-success status: warn for 5xx, debug for 4xx, with fields `status`, `code`/`type`, and a bounded, control-escaped `error.message`; never log the raw upstream body (F5). Confirm whether the `gateway/logging` redaction layer (`crates/gateway/logging/src/redact.rs`) applies to these tracing fields; if it does not, keep to structured code/type plus the escaped message. Note this post is shared by chat, embeddings, rerank, speech, and streaming - the broad application is intended. Cover with the existing `upstream.rs` test harness patterns. From 850ddedd92573f613e3d0c196959095a8a104cb4 Mon Sep 17 00:00:00 2001 From: Vinnie Falco Date: Tue, 22 Sep 2026 08:46:41 -0700 Subject: [PATCH 03/19] Close plan: replayed tool calls Plan: vibe/2026-09-22-1-replayed-tool-calls.md --- vibe/ACTIVE | 1 - 1 file changed, 1 deletion(-) delete mode 100644 vibe/ACTIVE diff --git a/vibe/ACTIVE b/vibe/ACTIVE deleted file mode 100644 index 5c2f5b18..00000000 --- a/vibe/ACTIVE +++ /dev/null @@ -1 +0,0 @@ -vibe/2026-09-22-1-replayed-tool-calls.md From cb8d437f28bbbae96ec0dabde042b656dbd18f84 Mon Sep 17 00:00:00 2001 From: Vinnie Falco Date: Tue, 22 Sep 2026 12:02:46 -0700 Subject: [PATCH 04/19] =?UTF-8?q?=EF=BB=BFCarry=20a=20reply=20origin=20on?= =?UTF-8?q?=20model=20reply=20events?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An inference round emitted a separate infer_reply event whose payload was identical to assistant_reply, so every consumer had to learn a second kind and any wildcard match silently dropped it. Fold the distinction into the existing reply event as a typed origin, so one event covers both and a host that cares branches on origin. - Event::AssistantReply gains origin: ReplyOrigin (Chat | Infer), serde defaulted to Chat, so an old reader ignores it and a new reader parses old logs. - Emitter::assistant_reply takes the origin; Emitter::infer_reply is gone. - support::report_model_turn passes the origin; the chat path emits Chat and the infer path emits Infer, through one shared reporting sequence. - The session sink settles and reply-stamps the single reply variant. Plan: none --- Cargo.lock | 1 + crates/harness/sessions/Cargo.toml | 4 +- crates/harness/sessions/src/session-tests.rs | 79 +++++ crates/harness/sessions/src/session.rs | 8 +- .../sessions/tests/it/session-infer.rs | 288 ++++++++++++++++++ crates/harness/sessions/tests/it/session.rs | 14 + .../src/execute/scheduler/chat.rs | 101 ++---- .../src/execute/support.rs | 104 ++++++- .../src/execute/tests/chat_arm.rs | 11 +- .../src/execute/tests/debug_and_counts.rs | 187 ++++++++++++ .../src/execute/tools.rs | 35 +-- .../test_support/recording-forward-tests.rs | 53 +++- .../src/test_support/recording-forward.rs | 2 + .../src/test_support/recording.rs | 6 +- crates/promptforge-api-types/src/emitter.rs | 10 +- .../promptforge-api-types/src/event-tests.rs | 79 ++++- crates/promptforge-api-types/src/event.rs | 27 +- .../lua/src/protocol/tests/answer.rs | 1 + crates/workshop/protocol/tests/it/fixture.rs | 1 + .../server/src/agents/status-tests.rs | 1 + 20 files changed, 884 insertions(+), 128 deletions(-) create mode 100644 crates/harness/sessions/src/session-tests.rs create mode 100644 crates/harness/sessions/tests/it/session-infer.rs diff --git a/Cargo.lock b/Cargo.lock index 8a8d6fd4..6f6ae8ea 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2668,6 +2668,7 @@ dependencies = [ name = "harness-sessions" version = "0.3.0" dependencies = [ + "axum", "harness-capabilities", "harness-log", "harness-models", diff --git a/crates/harness/sessions/Cargo.toml b/crates/harness/sessions/Cargo.toml index da943f45..e05885ca 100644 --- a/crates/harness/sessions/Cargo.toml +++ b/crates/harness/sessions/Cargo.toml @@ -47,7 +47,9 @@ tracing.workspace = true workspace-hack.workspace = true [dev-dependencies] -# The tag fixture the input suite spawns its suspended waits under. +# The loopback mock gateway the mixed-session suite drives real model rounds +# against, and the tag fixture its server is spawned under. +axum.workspace = true harness-runner = { workspace = true, features = ["test-support"] } # The discovery and session suites seed an agents directory on disk. tempfile.workspace = true diff --git a/crates/harness/sessions/src/session-tests.rs b/crates/harness/sessions/src/session-tests.rs new file mode 100644 index 00000000..ee222044 --- /dev/null +++ b/crates/harness/sessions/src/session-tests.rs @@ -0,0 +1,79 @@ +//! Tests for the reply-id stamp rule: the one rule a live event and a +//! transcript read share, so a reply of either origin - a user-facing chat +//! turn or a programmatic inference round - settles and advances the round +//! the same way. + +use promptforge_api_types::event::ReplyOrigin; +use promptforge_api_types::ids::Provenance; + +use super::*; + +/// The coordinates every event carries; only the kind and payload matter +/// to the stamp rule. +fn at(execution: &str) -> (String, String, Provenance) { + ( + execution.to_owned(), + "Section".to_owned(), + Provenance { + task: "0".parse().unwrap(), + seq: 0, + }, + ) +} + +#[test] +fn a_reply_of_either_origin_stamps_the_current_round_and_advances() { + for origin in [ReplyOrigin::Chat, ReplyOrigin::Infer] { + let mut rounds_seen = 3u64; + let (execution, section, provenance) = at("run-1"); + let reply = reply_stamp( + &Event::AssistantReply { + execution, + section, + provenance, + turn: 3, + text: "42".to_owned(), + finish_reason: Some("stop".to_owned()), + model: "some-model".to_owned(), + metrics: None, + origin, + }, + &mut rounds_seen, + ); + assert_eq!( + reply, + Some(3), + "an {origin:?} reply carries the round it was produced under" + ); + assert_eq!( + rounds_seen, 4, + "a settled {origin:?} round advances the count for the next round" + ); + } +} + +#[test] +fn thinking_stamps_the_current_round_without_advancing() { + let mut rounds_seen = 3u64; + let (execution, section, provenance) = at("run-1"); + let reply = reply_stamp( + &Event::Thinking { + execution, + section, + provenance, + turn: 3, + model: "some-model".to_owned(), + text: "weighing options".to_owned(), + }, + &mut rounds_seen, + ); + assert_eq!( + reply, + Some(3), + "thinking is stamped with the round it belongs to" + ); + assert_eq!( + rounds_seen, 3, + "thinking does not settle a round, so the count is unchanged" + ); +} diff --git a/crates/harness/sessions/src/session.rs b/crates/harness/sessions/src/session.rs index e5266e2a..03d65f96 100644 --- a/crates/harness/sessions/src/session.rs +++ b/crates/harness/sessions/src/session.rs @@ -441,7 +441,9 @@ impl SessionCore { self.lifecycle.settle_current_turn(); self.report(kind, format!("{boundary} in agent `{section}`")); } - Event::AssistantReply { .. } => self.lifecycle.settle_current_turn(), + Event::AssistantReply { .. } => { + self.lifecycle.settle_current_turn(); + } _ => {} } // The sink is called from one task at a time (the run's loop), so @@ -476,3 +478,7 @@ pub fn reply_stamp(event: &Event, rounds_seen: &mut u64) -> Option { _ => None, } } + +#[cfg(test)] +#[path = "session-tests.rs"] +mod tests; diff --git a/crates/harness/sessions/tests/it/session-infer.rs b/crates/harness/sessions/tests/it/session-infer.rs new file mode 100644 index 00000000..a8b9209f --- /dev/null +++ b/crates/harness/sessions/tests/it/session-infer.rs @@ -0,0 +1,288 @@ +//! The mock-gateway session round trip: a model-backed agent program drives +//! real inference- and chat-origin `assistant_reply` events through the +//! harness, and the tool-less infer reproduction pins the reply between the +//! completed turn and the section chunk success. + +use super::*; + +use axum::Router; +use axum::http::header::CONTENT_TYPE; +use axum::routing::{get, post}; +use harness_runner::spawn::spawn_tagged; +use harness_runner::test_support::mock_tag; + +/// A session program that infers once, then chats once: the mixed +/// model-round sequence the reply-id rule must number in order. The second +/// `user_input()` parks the run between the two rounds, so the infer reply +/// settles the accepted turn before any chat round exists. +const MIXED: &str = "---\nname: mixed\ndescription: infers then chats\npromptforge: 0\n\ + models:\n writer: {}\n---\n\n\ + # Mixed\n\n```lua\nmodels.default('writer')\n```\n\n\ + ## Only\n\n```lua\n\ + local first = user_input()\n\ + local inferred = models.infer(first)\n\ + local second = user_input()\n\ + local msgs = messages.new()\n\ + msgs:user(second)\n\ + models.loop(msgs)\n\ + return inferred .. '|' .. msgs[#msgs].content\n\ + ```\n"; + +/// The model id the mock gateway advertises and the catalog binds. +const MOCK_MODEL: &str = "mock-model"; + +/// The reply the mock gateway streams for every round. +const MOCK_REPLY: &str = "from the mock"; + +/// One round's stream: the reply in one content chunk, then a stop. +fn reply_stream() -> String { + let mut body = String::new(); + for event in [ + serde_json::json!({ + "model": MOCK_MODEL, + "choices": [{ "index": 0, "delta": { "content": MOCK_REPLY }, "finish_reason": null }] + }), + serde_json::json!({ + "model": MOCK_MODEL, + "choices": [{ "index": 0, "delta": {}, "finish_reason": "stop" }] + }), + ] { + body.push_str("data: "); + body.push_str(&event.to_string()); + body.push_str("\n\n"); + } + body.push_str("data: [DONE]\n\n"); + body +} + +/// The catalog the mock gateway serves at `GET /v1/models`: the one +/// inference model the harness binds its roles to. +fn catalog_body() -> serde_json::Value { + serde_json::json!({ + "data": [{ + "id": MOCK_MODEL, + "description": "the mock model", + "context": 131_072, + "thinking": "switchable", + }] + }) +} + +/// Serves the model catalog and a streaming chat completion on a loopback +/// port, returning the base URL to bind a harness to. +async fn mock_gateway() -> String { + async fn models() -> axum::Json { + axum::Json(catalog_body()) + } + async fn completions() -> ([(axum::http::HeaderName, &'static str); 1], String) { + ([(CONTENT_TYPE, "text/event-stream")], reply_stream()) + } + let app = Router::new() + .route("/v1/models", get(models)) + .route("/v1/chat/completions", post(completions)); + let listener = tokio::net::TcpListener::bind("127.0.0.1:0").await.unwrap(); + let addr = listener.local_addr().unwrap(); + spawn_tagged(mock_tag(), async move { + axum::serve(listener, app).await.unwrap(); + }); + format!("http://{addr}") +} + +/// A harness over a fresh `/agents` directory holding `name.md` with +/// `program`, bound to `base_url` with a catalog whose one chat-capable entry +/// names the mock gateway's model. +fn harness_for(dir: &Path, base_url: &str, name: &str, program: &str) -> Harness { + let agents = dir.join("agents"); + std::fs::create_dir_all(&agents).unwrap(); + std::fs::write(agents.join(format!("{name}.md")), program).unwrap(); + let harness = Harness::new(HarnessConfig { + agents_path: agents, + state_dir: dir.join("state"), + }); + harness.set_gateway(GatewayBinding { + base_url: base_url.to_owned(), + key: "k".to_owned(), + generation: 1, + }); + harness.set_catalog(CatalogBinding { + generation: 1, + models: vec![serde_json::json!({ "kind": "chat", "id": MOCK_MODEL })], + }); + harness +} + +/// A harness holding `mixed.md`: [`harness_for`] with [`MIXED`]. +fn mixed_harness(dir: &Path, base_url: &str) -> Harness { + harness_for(dir, base_url, "mixed", MIXED) +} + +/// A session program whose only model call is a tool-less `models.infer`: +/// the report's reproduction prompt. It parks on nothing, so it runs to +/// completion and the transcript holds its whole event stream. +const INFERS: &str = "---\nname: infers\ndescription: infers only\npromptforge: 0\n\ + models:\n writer: {}\n---\n\n\ + # Infers\n\n```lua\nmodels.default('writer')\n```\n\n\ + ## Only\n\n```lua\nreturn models.infer('prose')\n```\n"; + +/// A harness holding `infers.md` that makes one tool-less infer call and +/// returns: [`harness_for`] with [`INFERS`]. +fn infer_harness(dir: &Path, base_url: &str) -> Harness { + harness_for(dir, base_url, "infers", INFERS) +} + +#[tokio::test] +async fn a_mixed_infer_then_chat_session_numbers_each_reply_in_round_order() { + let dir = tempfile::tempdir().unwrap(); + let base = mock_gateway().await; + let harness = mixed_harness(dir.path(), &base); + let session = launch_agent(&harness, "mixed").await; + let mut waits = session.subscribe_waits(); + let first = required_token(&mut waits).await; + + session + .send_input(&first, "first".to_owned(), || {}) + .unwrap(); + // The infer reply precedes this second question; the chat reply follows + // the second answer. + let second = required_token(&mut waits).await; + session + .send_input(&second, "second".to_owned(), || {}) + .unwrap(); + + // The program returned after the chat: the session ends on its own. + wait_for(&session, SessionState::Closed).await; + + let events = session.transcript(0).await.unwrap(); + let replies: Vec<(&str, Option)> = events + .iter() + .filter_map(|event| { + let kind = event.event.get("kind")?.as_str()?; + if kind != "assistant_reply" { + return None; + } + let origin = event + .event + .get("origin") + .and_then(serde_json::Value::as_str)?; + Some((origin, event.reply)) + }) + .collect(); + assert_eq!( + replies, + vec![("infer", Some(0)), ("chat", Some(1))], + "the infer reply takes round 0 and advances; the chat reply takes round 1" + ); +} + +#[tokio::test] +async fn an_infer_reply_settles_the_accepted_turn_so_a_new_catalog_retires_the_run() { + let dir = tempfile::tempdir().unwrap(); + let base = mock_gateway().await; + let harness = mixed_harness(dir.path(), &base); + let session = launch_agent(&harness, "mixed").await; + let mut waits = session.subscribe_waits(); + let first = required_token(&mut waits).await; + + // The answer arms the accepted turn; the infer reply settles it and the + // program parks on its second question. + session + .send_input(&first, "first".to_owned(), || {}) + .unwrap(); + let second = required_token(&mut waits).await; + assert_ne!(second, first, "wait tokens are single-use"); + + // The infer settled the accepted turn, so a replacement catalog retires + // the run at once. Were an infer-origin `AssistantReply` outside the + // settle arm, the still open accepted turn would defer the retirement + // onto a settlement that never arrives, and this test would time out on + // the cancelled frame. + harness.set_catalog(CatalogBinding { + generation: 2, + models: vec![serde_json::json!({ + "kind": "chat", + "id": MOCK_MODEL, + "description": "other", + })], + }); + cancelled_frame(&mut waits, &second).await; + let third = required_token(&mut waits).await; + assert_ne!(third, second, "the relaunch parks under a fresh token"); + assert_eq!( + session.run_ids().len(), + 2, + "the settled infer turn let the new catalog retire the run" + ); + + assert!(harness.close(session.id())); + wait_for(&session, SessionState::Closed).await; +} + +#[tokio::test] +async fn a_tool_less_infer_reply_sits_between_the_completed_turn_and_the_chunk_success() { + // The report's reproduction: a section whose only model call is + // `return models.infer(prose)`. Its session stream must carry the + // programmatic reply after the completed boundary and before the + // section's chunk success, with the model's text and an infer origin + // (not a chat turn). + let dir = tempfile::tempdir().unwrap(); + let base = mock_gateway().await; + let harness = infer_harness(dir.path(), &base); + let session = launch_agent(&harness, "infers").await; + wait_for(&session, SessionState::Closed).await; + + let events = session.transcript(0).await.unwrap(); + let kinds: Vec<&str> = events + .iter() + .filter_map(|event| event.event.get("kind")?.as_str()) + .collect(); + let completed = kinds + .iter() + .position(|kind| *kind == "model_turn_completed") + .expect("the model turn completed"); + let reply = kinds + .iter() + .position(|kind| *kind == "assistant_reply") + .expect("the tool-less infer reported a reply"); + assert!( + completed < reply, + "the infer reply follows the completed turn: {kinds:?}" + ); + assert!( + kinds[reply + 1..].contains(&"lua_chunk_succeeded"), + "the infer reply precedes the section's chunk success: {kinds:?}" + ); + let reply_event = events + .iter() + .find(|event| { + event.event.get("kind").and_then(serde_json::Value::as_str) == Some("assistant_reply") + }) + .expect("the tool-less infer reported a reply"); + let text = reply_event + .event + .get("text") + .and_then(serde_json::Value::as_str); + assert_eq!( + text, + Some(MOCK_REPLY), + "the infer reply carries the model text" + ); + assert_eq!( + reply_event + .event + .get("origin") + .and_then(serde_json::Value::as_str), + Some("infer"), + "the infer round's reply is an assistant reply tagged infer: {:?}", + reply_event.event + ); + assert!( + !events.iter().any(|event| { + event + .event + .get("origin") + .and_then(serde_json::Value::as_str) + == Some("chat") + }), + "a tool-less infer emits no chat-origin reply: {kinds:?}" + ); +} diff --git a/crates/harness/sessions/tests/it/session.rs b/crates/harness/sessions/tests/it/session.rs index ec24f1ee..909cc9df 100644 --- a/crates/harness/sessions/tests/it/session.rs +++ b/crates/harness/sessions/tests/it/session.rs @@ -25,6 +25,9 @@ use tokio::sync::broadcast; #[path = "session-close.rs"] mod close; +#[path = "session-infer.rs"] +mod infer; + /// A prompt that parks on operator input and returns it. const ASKS: &str = "---\nname: asks\ndescription: asks the operator\npromptforge: 0\n---\n\n\ # Asks\n\n## Only\n\n```lua\nreturn user_input()\n```\n"; @@ -77,6 +80,17 @@ async fn launch(harness: &Harness) -> Session { .expect("the discovered agent launches") } +/// Launches the discovered agent named `agent`. +async fn launch_agent(harness: &Harness, agent: &str) -> Session { + harness + .launch(LaunchRequest { + agent: agent.to_owned(), + args: String::new(), + }) + .await + .expect("the discovered agent launches") +} + /// Waits for the run to park on its input wait. async fn required_token(waits: &mut broadcast::Receiver) -> String { let frame = tokio::time::timeout(PATIENCE, waits.recv()) diff --git a/crates/promptforge-api-runtime/src/execute/scheduler/chat.rs b/crates/promptforge-api-runtime/src/execute/scheduler/chat.rs index 19d60bcb..dc94066a 100644 --- a/crates/promptforge-api-runtime/src/execute/scheduler/chat.rs +++ b/crates/promptforge-api-runtime/src/execute/scheduler/chat.rs @@ -20,12 +20,12 @@ use std::collections::BTreeMap; use std::sync::Arc; use std::sync::atomic::AtomicU32; -use promptforge_api_types::metrics::{CallMetrics, ToolCallEvent}; +use promptforge_api_types::metrics::ToolCallEvent; use crate::execute::protocol::{Answer, ChatResult}; use crate::execute::run::Effect; use crate::execute::scope::{DispatchTarget, prepare_effective_scope}; -use crate::execute::support::advance_turn; +use crate::execute::support::{Served, advance_turn, report_model_turn}; use crate::lua::{ MessageRecord, OverflowReason, current_tool_bindings, is_context_overflow, precheck, project_messages, resolve_model_binding, @@ -34,6 +34,7 @@ use crate::model::ModelBinding; use crate::model::{Completion, CompletionResult, ToolCall}; use crate::{Error, Result}; use promptforge_api_types::emitter::Emitter; +use promptforge_api_types::event::ReplyOrigin; use promptforge_api_types::event::lifecycle; use super::builtins::{advertise_task_builtins, scope_halves, task_allowlist}; @@ -54,22 +55,6 @@ fn overflow_result(reason: OverflowReason) -> ChatResult { } } -/// Assembles one round's [`CallMetrics`] from everything the completion -/// measured, or `None` when nothing was measured. -fn call_metrics(completion: &Completion) -> Option { - let metrics = CallMetrics { - usage: completion.usage().cloned(), - llama: completion.llama_timings().cloned(), - vllm: completion.vllm_metrics().cloned(), - client: completion.client_timing().cloned(), - }; - let measured = metrics.usage.is_some() - || metrics.llama.is_some() - || metrics.vllm.is_some() - || metrics.client.is_some(); - measured.then_some(metrics) -} - /// How one `chat` dispatch resolved: a round issued as an effect and /// parked on the pending table, or an answer settled without leaving (the /// precheck overflow). @@ -227,7 +212,7 @@ impl Scheduler { let turn = advance_turn(&round.turns); let (outcome, served) = round.served(*completion, turn); let result = match outcome { - CompletionResult::Text(text) => Ok(round.text_reply(&served, turn, text)), + CompletionResult::Text(text) => Ok(Round::text_reply(&served, text)), CompletionResult::ToolCalls(calls) => { // The scope is recorded before the round is spawned; its // absence is a scheduler fault, never an author-visible @@ -259,15 +244,6 @@ struct Round { turns: Arc, } -/// What a served completion reports once the turn has advanced and the -/// completion observation has fired: the pieces both the text and the -/// tool-call arms pass into the round's answer. -struct Served { - finish_reason: Option, - model: String, - metrics: Option, -} - impl Round { /// Classifies a round that produced no completion. A provider context /// rejection is the overflow answer under a failed turn. An empty reply @@ -311,64 +287,25 @@ impl Round { } } - /// Reports a served completion's round-level events - the debug - /// capture pair, the completion event, and the thinking side channel - - /// and dissolves the completion into its outcome and what the answer - /// arms report beside it. + /// Reports a served completion's round-level events through the shared + /// round report - the debug capture pair, the completed boundary, the + /// thinking side channel, the `length` truncation observation, and the + /// `assistant_reply` content report - and dissolves the completion into + /// its outcome and the metadata the answer arms carry beside it. fn served(&self, completion: Completion, turn: u32) -> (CompletionResult, Served) { - // Extracted before the debug capture, which moves the request body - // out of the completion. - let metrics = call_metrics(&completion); - let model = completion.model().to_owned(); - let thinking = completion - .reasoning_content() - .filter(|text| !text.is_empty()) - .map(str::to_owned); - let finish_reason = completion.finish_reason().map(str::to_owned); - if self.emitter.captures_debug() { - self.emitter - .request(&self.section, turn, completion.request_body); - self.emitter.response( - &self.section, - turn, - completion.response_body.clone(), - completion.finish_reason.clone(), - completion.reasoning_content.clone(), - ); - } - self.emitter - .report(&self.section, lifecycle::MODEL_TURN_COMPLETED); - // The content reports every host transcript is built from: the - // thinking side channel first, then the reply or the tool-call - // batch, each with model and metrics. - if let Some(thinking) = &thinking { - self.emitter.thinking(&self.section, turn, &model, thinking); - } - ( - completion.result, - Served { - finish_reason, - model, - metrics, - }, + report_model_turn( + &self.emitter, + &self.section, + turn, + completion, + ReplyOrigin::Chat, ) } - /// Reports a text reply (with the truncation observation on a `length` - /// finish) and builds its answer. - fn text_reply(&self, served: &Served, turn: u32, text: String) -> ChatResult { - if served.finish_reason.as_deref() == Some("length") { - self.emitter - .report(&self.section, lifecycle::MODEL_TURN_TRUNCATED); - } - self.emitter.assistant_reply( - &self.section, - turn, - &text, - served.finish_reason.as_deref(), - &served.model, - served.metrics.as_ref(), - ); + /// Builds a text reply's answer. Its `assistant_reply` content report, + /// with the truncation observation on a `length` finish, fired from + /// [`report_model_turn`]. + fn text_reply(served: &Served, text: String) -> ChatResult { ChatResult { overflow: false, overflow_reason: None, diff --git a/crates/promptforge-api-runtime/src/execute/support.rs b/crates/promptforge-api-runtime/src/execute/support.rs index 405b5cd4..1d1540df 100644 --- a/crates/promptforge-api-runtime/src/execute/support.rs +++ b/crates/promptforge-api-runtime/src/execute/support.rs @@ -1,8 +1,15 @@ -//! Cross-cutting run helpers: the turn counter, the `sys` JSON, and the -//! shared run constants. +//! Cross-cutting run helpers: the turn counter, the `sys` JSON, the round +//! metrics, the shared round report, and the shared run constants. use std::sync::atomic::{AtomicU32, Ordering}; +use promptforge_api_types::emitter::Emitter; +use promptforge_api_types::event::ReplyOrigin; +use promptforge_api_types::event::lifecycle; +use promptforge_api_types::metrics::CallMetrics; + +use crate::model::{Completion, CompletionResult}; + /// Maximum nested `call()` depth (inclusive of the first call). pub(crate) const MAX_CALL_DEPTH: usize = 8; @@ -52,3 +59,96 @@ pub(crate) fn sys_json( "section_count": section_count, }) } + +/// Assembles one round's [`CallMetrics`] from everything the completion +/// measured, or `None` when nothing was measured. +pub(crate) fn call_metrics(completion: &Completion) -> Option { + let metrics = CallMetrics { + usage: completion.usage().cloned(), + llama: completion.llama_timings().cloned(), + vllm: completion.vllm_metrics().cloned(), + client: completion.client_timing().cloned(), + }; + let measured = metrics.usage.is_some() + || metrics.llama.is_some() + || metrics.vllm.is_some() + || metrics.client.is_some(); + measured.then_some(metrics) +} + +/// What a served completion reports once the turn has advanced and the +/// round-level events have fired: the pieces the chat arm's answer arms +/// carry alongside the outcome. The nested-inference arm ignores it. +pub(crate) struct Served { + pub(crate) finish_reason: Option, + pub(crate) model: String, + pub(crate) metrics: Option, +} + +/// Reports one served completion's round-level events and returns its +/// outcome beside the metadata an answer needs. +/// +/// The sequence is fixed and lives here for both the chat and the nested +/// inference paths: the debug request/response pair, `MODEL_TURN_COMPLETED`, +/// the thinking side channel, the `length` truncation observation, then +/// exactly one `assistant_reply` content report carrying `origin` - the +/// chat arm's [`ReplyOrigin::Chat`] or the nested-inference arm's +/// [`ReplyOrigin::Infer`]. A non-text outcome fires the shared prefix and +/// no content report. +pub(crate) fn report_model_turn( + emitter: &Emitter, + section: &str, + turn: u32, + completion: Completion, + origin: ReplyOrigin, +) -> (CompletionResult, Served) { + // Extracted before the debug capture, which moves the request body out + // of the completion. + let metrics = call_metrics(&completion); + let model = completion.model().to_owned(); + let thinking = completion + .reasoning_content() + .filter(|text| !text.is_empty()) + .map(str::to_owned); + let finish_reason = completion.finish_reason().map(str::to_owned); + if emitter.captures_debug() { + emitter.request(section, turn, completion.request_body); + emitter.response( + section, + turn, + completion.response_body.clone(), + completion.finish_reason.clone(), + completion.reasoning_content.clone(), + ); + } + emitter.report(section, lifecycle::MODEL_TURN_COMPLETED); + // The content reports every host transcript is built from: the + // thinking side channel first, then the reply, each with model and + // metrics. + if let Some(thinking) = &thinking { + emitter.thinking(section, turn, &model, thinking); + } + let served = Served { + finish_reason, + model, + metrics, + }; + let outcome = completion.result; + if let CompletionResult::Text(text) = &outcome { + if served.finish_reason.as_deref() == Some("length") { + emitter.report(section, lifecycle::MODEL_TURN_TRUNCATED); + } + let finish_reason = served.finish_reason.as_deref(); + let metrics = served.metrics.as_ref(); + emitter.assistant_reply( + section, + turn, + text, + finish_reason, + &served.model, + metrics, + origin, + ); + } + (outcome, served) +} diff --git a/crates/promptforge-api-runtime/src/execute/tests/chat_arm.rs b/crates/promptforge-api-runtime/src/execute/tests/chat_arm.rs index c9d14f86..4f4f302f 100644 --- a/crates/promptforge-api-runtime/src/execute/tests/chat_arm.rs +++ b/crates/promptforge-api-runtime/src/execute/tests/chat_arm.rs @@ -10,6 +10,7 @@ use super::models_loop::{echo_tools, loop_models, loop_prompt}; use super::*; use crate::lua::ToolSet; use crate::test_support::tokio_driver::TokioDriver; +use promptforge_api_types::event::ReplyOrigin; use promptforge_api_types::metrics::{CallMetrics, ToolCallEvent}; /// Records every observation and every content report as one rendered @@ -66,9 +67,10 @@ impl Observer for RoundRecorder { finish_reason: Option<&str>, model: &str, metrics: Option<&CallMetrics>, + origin: ReplyOrigin, ) { self.push(format!( - "{section}: reply chain={chain_id} depth={depth} turn={turn} text={text} \ + "{section}: reply origin={origin:?} chain={chain_id} depth={depth} turn={turn} text={text} \ finish={finish_reason:?} model={model} metrics={}", metrics.is_some() )); @@ -211,6 +213,13 @@ async fn a_chat_round_reports_the_same_sequence_as_the_rust_loop_for_a_text_repl reference, "the chat arm reports exactly the loop's one-round sequence" ); + let chat_lines = chat_recorder.lines(); + assert!( + chat_lines + .iter() + .any(|line| line.contains("reply origin=Chat")), + "the chat arm reports a chat-origin reply, pinning the emit site's `ReplyOrigin::Chat`: {chat_lines:?}" + ); assert_eq!( chat_gateway.requests()[0]["tools"][0]["function"]["name"], "echo", diff --git a/crates/promptforge-api-runtime/src/execute/tests/debug_and_counts.rs b/crates/promptforge-api-runtime/src/execute/tests/debug_and_counts.rs index 31bbba6f..550c2a23 100644 --- a/crates/promptforge-api-runtime/src/execute/tests/debug_and_counts.rs +++ b/crates/promptforge-api-runtime/src/execute/tests/debug_and_counts.rs @@ -3,6 +3,9 @@ use super::run; use super::*; +use promptforge_api_types::event::ReplyOrigin; +use promptforge_api_types::metrics::CallMetrics; + #[tokio::test] async fn debug_capture_receives_request_and_response_when_set() { let gateway = ScriptedGateway::start(vec![resp_text("hello from the mock")]).await; @@ -335,3 +338,187 @@ async fn handle_infer_returns_text_without_touching_reply_or_sys() { "handle-form infer advertises no tools: {body}" ); } + +// --- Infer-round reporting: `assistant_reply` with `origin = infer` --- + +/// Records an infer round's boundary observations and content hooks as one +/// line each, so the whole sequence is asserted rather than a count: the +/// infer turn must read `model_turn_completed` -> (`thinking` when the +/// backend supplied reasoning) -> `assistant_reply` with `origin = +/// Infer`. +#[derive(Default)] +struct InferRoundRecorder(Mutex>); + +impl InferRoundRecorder { + fn push(&self, line: String) { + self.0 + .lock() + .expect("the infer round recorder mutex is not poisoned") + .push(line); + } + + fn lines(&self) -> Vec { + self.0 + .lock() + .expect("the infer round recorder mutex is not poisoned") + .clone() + } +} + +impl Observer for InferRoundRecorder { + fn observe(&self, _execution: &str, section: &str, event: Observation) { + self.push(format!("{section}: {event}")); + } + + fn on_thinking( + &self, + _execution: &str, + section: &str, + _chain_id: u32, + _depth: u32, + turn: u32, + model: &str, + text: &str, + ) { + self.push(format!( + "{section}: thinking turn={turn} model={model} text={text}" + )); + } + + fn on_assistant_reply( + &self, + _execution: &str, + section: &str, + _chain_id: u32, + _depth: u32, + turn: u32, + text: &str, + finish_reason: Option<&str>, + model: &str, + metrics: Option<&CallMetrics>, + origin: ReplyOrigin, + ) { + self.push(format!( + "{section}: assistant_reply origin={origin:?} turn={turn} text={text} finish={finish_reason:?} model={model} metrics={}", + metrics.is_some() + )); + } +} + +/// The index of the first recorded line containing `needle`, panicking with +/// the whole sequence when it is absent. +fn line_index(lines: &[String], needle: &str) -> usize { + lines + .iter() + .position(|line| line.contains(needle)) + .unwrap_or_else(|| panic!("no recorded line contains {needle:?}: {lines:#?}")) +} + +/// Runs one handle-form `models.infer` round scripted with `reply` and +/// returns the run's output beside every line the recorder saw. +async fn run_infer_round(reply: GatewayReply) -> (String, Vec) { + let gateway = ScriptedGateway::start(vec![reply]).await; + let addr = gateway.addr(); + let recorder = Arc::new(InferRoundRecorder::default()); + let md = "---\nname: t\ndescription: d\npromptforge: 0\nmodels:\n writer: {}\n---\n\n\ + # Test prompt\n\n```lua shared\n\ + writer = models.default('writer')\n```\n\n\ + ## Only\n\n\ + ```lua\n\ + return models.infer(writer, 'say hello')\n\ + ```\n"; + let prompt = bound_with_tools(md); + let out = run( + &prompt, + "", + &[], + &TestStore::new(), + RunOptions { + execution: EXECUTION, + observer: Arc::clone(&recorder) as Arc, + client: Some(gateway_client(addr)), + debug: None, + }, + ) + .await + .expect("handle-form infer must return text"); + (out, recorder.lines()) +} + +/// A text reply carrying a `reasoning_content` side channel. +fn resp_text_with_reasoning(content: &str, reasoning: &str) -> GatewayReply { + GatewayReply::Json(json!({ + "choices": [{ + "message": { + "role": "assistant", + "reasoning_content": reasoning, + "content": content, + } + }] + })) +} + +#[tokio::test] +async fn infer_round_reports_model_turn_completed_then_an_infer_origin_assistant_reply() { + // The infer turn's reporting order: the completed boundary, then one + // `assistant_reply` tagged `origin = Infer`. + let (out, lines) = run_infer_round(resp_text("pong")).await; + assert_eq!(out, "pong"); + + let completed = line_index(&lines, "Model turn completed"); + let reply = line_index(&lines, "assistant_reply"); + assert_eq!( + reply, + completed + 1, + "the infer reply must immediately follow the completed turn: {lines:#?}" + ); + assert!( + lines[reply].contains("origin=Infer") + && lines[reply].contains("text=pong") + && lines[reply].contains("turn=1"), + "the infer reply must carry the round's infer origin, text, and turn: {lines:#?}" + ); + assert!( + !lines.iter().any(|line| line.contains("origin=Chat")), + "an infer round must not report a chat-origin reply: {lines:#?}" + ); + assert!( + !lines.iter().any(|line| line.contains("thinking")), + "a reply with no reasoning must not report a thinking block: {lines:#?}" + ); +} + +#[tokio::test] +async fn infer_round_reports_thinking_between_the_completed_turn_and_the_reply() { + // The thinking parity path: reasoning the backend supplied reaches the + // observer as `thinking`, ordered after the completed boundary and + // before the infer-origin `assistant_reply`. + let (out, lines) = run_infer_round(resp_text_with_reasoning("pong", "let me think")).await; + assert_eq!(out, "pong"); + + let completed = line_index(&lines, "Model turn completed"); + let thinking = line_index(&lines, "thinking"); + let reply = line_index(&lines, "assistant_reply"); + assert_eq!( + thinking, + completed + 1, + "thinking must follow the completed turn: {lines:#?}" + ); + assert_eq!( + reply, + completed + 2, + "the infer reply must follow the thinking block: {lines:#?}" + ); + assert!( + lines[thinking].contains("text=let me think") && lines[thinking].contains("turn=1"), + "the thinking block must carry the reasoning text and turn: {lines:#?}" + ); + assert!( + lines[reply].contains("text=pong") && lines[reply].contains("origin=Infer"), + "the infer reply must carry the round's text and infer origin: {lines:#?}" + ); + assert!( + !lines.iter().any(|line| line.contains("origin=Chat")), + "an infer round must not report a chat-origin reply: {lines:#?}" + ); +} diff --git a/crates/promptforge-api-runtime/src/execute/tools.rs b/crates/promptforge-api-runtime/src/execute/tools.rs index 9db99e28..c7c13d3b 100644 --- a/crates/promptforge-api-runtime/src/execute/tools.rs +++ b/crates/promptforge-api-runtime/src/execute/tools.rs @@ -14,15 +14,18 @@ use std::sync::atomic::AtomicU32; use crate::Error; use crate::model::{Completion, CompletionError, CompletionResult}; +use promptforge_api_types::event::ReplyOrigin; use promptforge_api_types::event::lifecycle; -use super::support::advance_turn; +use super::support::{advance_turn, report_model_turn}; use promptforge_api_types::emitter::Emitter; -/// Reports one completed infer round exactly like a single prose round and -/// renders its text: the turn advance, the debug capture pair, the -/// completion and truncation events, and the no-tools-advertised -/// violation check. +/// Reports one completed infer round through the shared round report and +/// renders its text. The report fires the turn advance's round events - the +/// debug capture pair, the completed boundary, the thinking side channel, +/// the `length` truncation observation - plus the [`Emitter::assistant_reply`] +/// content report tagged `origin = infer`. The no-tools-advertised +/// violation check is what keeps the round tool-free. fn accept_infer_completion( completion: Completion, emitter: &Emitter, @@ -30,25 +33,9 @@ fn accept_infer_completion( turns: &AtomicU32, ) -> Result { let turn = advance_turn(turns); - if emitter.captures_debug() { - emitter.request(section, turn, completion.request_body); - emitter.response( - section, - turn, - completion.response_body.clone(), - completion.finish_reason.clone(), - completion.reasoning_content.clone(), - ); - } - emitter.report(section, lifecycle::MODEL_TURN_COMPLETED); - - match completion.result { - CompletionResult::Text(text) => { - if completion.finish_reason.as_deref() == Some("length") { - emitter.report(section, lifecycle::MODEL_TURN_TRUNCATED); - } - Ok(text) - } + let (outcome, _) = report_model_turn(emitter, section, turn, completion, ReplyOrigin::Infer); + match outcome { + CompletionResult::Text(text) => Ok(text), // No tools were advertised, so a tool-call turn is a backend // protocol violation rather than something to dispatch. // `CompletionResult` is `#[non_exhaustive]` across the crate boundary: diff --git a/crates/promptforge-api-runtime/src/test_support/recording-forward-tests.rs b/crates/promptforge-api-runtime/src/test_support/recording-forward-tests.rs index fe7f70e8..41f82aa6 100644 --- a/crates/promptforge-api-runtime/src/test_support/recording-forward-tests.rs +++ b/crates/promptforge-api-runtime/src/test_support/recording-forward-tests.rs @@ -5,6 +5,7 @@ use std::sync::Mutex; +use promptforge_api_types::event::ReplyOrigin; use promptforge_api_types::ids::{AbandonReason, ChainId, Provenance, TaskId, TaskOrigin}; use promptforge_api_types::metrics::ToolCallEvent; @@ -43,12 +44,13 @@ impl Observer for Recorder { finish_reason: Option<&str>, model: &str, _metrics: Option<&promptforge_api_types::metrics::CallMetrics>, + origin: ReplyOrigin, ) { self.content .lock() .expect("the recorder mutex is not poisoned") .push(format!( - "{section}: reply chain={chain_id} depth={depth} turn={turn} text={text} finish={finish_reason:?} model={model}" + "{section}: reply origin={origin:?} chain={chain_id} depth={depth} turn={turn} text={text} finish={finish_reason:?} model={model}" )); } @@ -349,6 +351,21 @@ fn one_of_every_event_variant() -> Vec<(Event, Seam)> { finish_reason: Some("stop".to_owned()), model: "m".to_owned(), metrics: None, + origin: ReplyOrigin::Chat, + }, + Seam::Content, + ), + ( + Event::AssistantReply { + execution: "run".to_owned(), + section: "A".to_owned(), + provenance: provenance(), + turn: 1, + text: "inferred".to_owned(), + finish_reason: Some("stop".to_owned()), + model: "m".to_owned(), + metrics: None, + origin: ReplyOrigin::Infer, }, Seam::Content, ), @@ -482,6 +499,7 @@ fn each_event_group_reaches_its_seam_in_batch_order() { finish_reason: Some("stop".to_owned()), model: "m".to_owned(), metrics: None, + origin: ReplyOrigin::Chat, }, Event::UserInput { execution: "run".to_owned(), @@ -514,7 +532,8 @@ fn each_event_group_reaches_its_seam_in_batch_order() { assert_eq!( *recorder.content.lock().expect("not poisoned"), vec![ - "A: reply chain=0 depth=0 turn=1 text=hi finish=Some(\"stop\") model=m".to_owned(), + "A: reply origin=Chat chain=0 depth=0 turn=1 text=hi finish=Some(\"stop\") model=m" + .to_owned(), "A: input typed".to_owned(), ] ); @@ -524,6 +543,36 @@ fn each_event_group_reaches_its_seam_in_batch_order() { ); } +#[test] +fn a_reply_forwards_its_origin_to_the_observer() { + // A reply's provenance reaches the observer: dropping it, defaulting it + // to `Chat`, or routing it to a second kind would change this line. + let recorder = Recorder::default(); + forward( + vec![Event::AssistantReply { + execution: "run".to_owned(), + section: "A".to_owned(), + provenance: provenance(), + turn: 1, + text: "inferred".to_owned(), + finish_reason: Some("stop".to_owned()), + model: "m".to_owned(), + metrics: None, + origin: ReplyOrigin::Infer, + }], + &recorder, + None, + ); + assert_eq!( + *recorder.content.lock().expect("not poisoned"), + vec![ + "A: reply origin=Infer chain=0 depth=0 turn=1 text=inferred finish=Some(\"stop\") model=m" + .to_owned(), + ], + "the observer must see the reply's origin" + ); +} + #[test] fn debug_events_are_dropped_without_a_capture() { let recorder = Recorder::default(); diff --git a/crates/promptforge-api-runtime/src/test_support/recording-forward.rs b/crates/promptforge-api-runtime/src/test_support/recording-forward.rs index 67f8102a..5b8192ce 100644 --- a/crates/promptforge-api-runtime/src/test_support/recording-forward.rs +++ b/crates/promptforge-api-runtime/src/test_support/recording-forward.rs @@ -258,6 +258,7 @@ fn forward_content(event: Event, observer: &dyn Observer) { finish_reason, model, metrics, + origin, .. } => observer.on_assistant_reply( &execution, @@ -269,6 +270,7 @@ fn forward_content(event: Event, observer: &dyn Observer) { finish_reason.as_deref(), &model, metrics.as_ref(), + origin, ), Event::AssistantToolCalls { execution, diff --git a/crates/promptforge-api-runtime/src/test_support/recording.rs b/crates/promptforge-api-runtime/src/test_support/recording.rs index 49fe0054..279b8eff 100644 --- a/crates/promptforge-api-runtime/src/test_support/recording.rs +++ b/crates/promptforge-api-runtime/src/test_support/recording.rs @@ -21,6 +21,7 @@ use std::sync::{Mutex, PoisonError}; +use promptforge_api_types::event::ReplyOrigin; use promptforge_api_types::ids::TaskId; use promptforge_api_types::metrics::{CallMetrics, ToolCallEvent}; use serde_json::Value; @@ -44,7 +45,9 @@ pub trait Observer: Send + Sync { /// Records one typed [`Observation`] for `execution` and `section`. fn observe(&self, execution: &str, section: &str, event: Observation); - /// Records one completed assistant reply. + /// Records one completed assistant reply. `origin` is the reply's + /// provenance: [`ReplyOrigin::Chat`] for a user-facing turn, + /// [`ReplyOrigin::Infer`] for a programmatic inference round. #[expect(unused_variables, reason = "the default body discards the report")] fn on_assistant_reply( &self, @@ -57,6 +60,7 @@ pub trait Observer: Send + Sync { finish_reason: Option<&str>, model: &str, metrics: Option<&CallMetrics>, + origin: ReplyOrigin, ) { } diff --git a/crates/promptforge-api-types/src/emitter.rs b/crates/promptforge-api-types/src/emitter.rs index 6195f6ba..d522667a 100644 --- a/crates/promptforge-api-types/src/emitter.rs +++ b/crates/promptforge-api-types/src/emitter.rs @@ -27,6 +27,7 @@ use std::sync::{Arc, Mutex}; use serde_json::Value; use crate::event::Event; +use crate::event::ReplyOrigin; use crate::event::lifecycle::Lifecycle; use crate::ids::{ChainId, Provenance, TaskId}; use crate::metrics::{CallMetrics, ToolCallEvent}; @@ -268,7 +269,12 @@ impl Emitter { }); } - /// Reports one completed assistant reply. + /// Reports one completed assistant reply: a model round's text reply + /// with its [`ReplyOrigin`] provenance. + #[expect( + clippy::too_many_arguments, + reason = "the reply report names its full run coordinates, including the origin, in one call" + )] pub fn assistant_reply( &self, section: &str, @@ -277,6 +283,7 @@ impl Emitter { finish_reason: Option<&str>, model: &str, metrics: Option<&CallMetrics>, + origin: ReplyOrigin, ) { self.emit(section, |execution, section, provenance| { Event::AssistantReply { @@ -288,6 +295,7 @@ impl Emitter { finish_reason: finish_reason.map(str::to_owned), model: model.to_owned(), metrics: metrics.cloned(), + origin, } }); } diff --git a/crates/promptforge-api-types/src/event-tests.rs b/crates/promptforge-api-types/src/event-tests.rs index b23fb730..50bf4bca 100644 --- a/crates/promptforge-api-types/src/event-tests.rs +++ b/crates/promptforge-api-types/src/event-tests.rs @@ -3,6 +3,7 @@ use serde_json::json; use super::Event; +use super::ReplyOrigin; use crate::ids::{AbandonReason, Provenance, TaskId, TaskOrigin}; use crate::metrics::{CallMetrics, ToolCallEvent, Usage}; @@ -17,6 +18,21 @@ fn provenance(path: &str, seq: u32) -> Provenance { } } +fn sample_metrics() -> CallMetrics { + CallMetrics { + usage: Some(Usage { + prompt_tokens: 7, + completion_tokens: 3, + total_tokens: 10, + cached_tokens: None, + reasoning_tokens: None, + }), + llama: None, + vllm: None, + client: None, + } +} + fn round_trips(event: &Event) { let line = serde_json::to_string(event).expect("every event serializes"); assert!( @@ -71,18 +87,19 @@ fn one_variant_of_each_group_round_trips_through_serde() { text: "hello".to_owned(), finish_reason: Some("stop".to_owned()), model: "llama-3".to_owned(), - metrics: Some(CallMetrics { - usage: Some(Usage { - prompt_tokens: 7, - completion_tokens: 3, - total_tokens: 10, - cached_tokens: None, - reasoning_tokens: None, - }), - llama: None, - vllm: None, - client: None, - }), + metrics: Some(sample_metrics()), + origin: ReplyOrigin::Chat, + }); + round_trips(&Event::AssistantReply { + execution: "run-1".to_owned(), + section: "Gather".to_owned(), + provenance: provenance("0", 7), + turn: 2, + text: "hello".to_owned(), + finish_reason: Some("stop".to_owned()), + model: "llama-3".to_owned(), + metrics: Some(sample_metrics()), + origin: ReplyOrigin::Infer, }); round_trips(&Event::AssistantToolCalls { execution: "run-1".to_owned(), @@ -118,6 +135,44 @@ fn one_variant_of_each_group_round_trips_through_serde() { }); } +#[test] +fn a_reply_origin_defaults_to_chat() { + assert_eq!(ReplyOrigin::default(), ReplyOrigin::Chat); +} + +#[test] +fn a_reply_serializes_its_origin() { + let event = Event::AssistantReply { + execution: "run-1".to_owned(), + section: "Gather".to_owned(), + provenance: provenance("0", 20), + turn: 1, + text: "inferred".to_owned(), + finish_reason: None, + model: "llama-3".to_owned(), + metrics: None, + origin: ReplyOrigin::Infer, + }; + let line = serde_json::to_string(&event).expect("an event serializes"); + assert!( + line.contains(r#""origin":"infer""#), + "the origin must reach the wire: {line}" + ); +} + +#[test] +fn an_older_reply_without_origin_reads_back_as_chat() { + // Backward compatibility: a log line written before `origin` existed + // must still parse, defaulting to a chat reply. Without + // `#[serde(default)]` this deserialization fails. + let line = r#"{"kind":"assistant_reply","execution":"run-1","section":"Gather","provenance":{"task":"0","seq":1},"turn":1,"text":"hi","finish_reason":null,"model":"llama-3","metrics":null}"#; + let event: Event = serde_json::from_str(line).expect("an old reply parses"); + match event { + Event::AssistantReply { origin, .. } => assert_eq!(origin, ReplyOrigin::Chat), + other => panic!("expected an assistant reply, got {other:?}"), + } +} + #[test] fn a_serialized_event_is_tagged_by_kind_with_its_coordinates_beside_the_payload() { // The tag and the three coordinates are the log schema the harness diff --git a/crates/promptforge-api-types/src/event.rs b/crates/promptforge-api-types/src/event.rs index e80041d9..f2b6b0b6 100644 --- a/crates/promptforge-api-types/src/event.rs +++ b/crates/promptforge-api-types/src/event.rs @@ -134,6 +134,25 @@ macro_rules! events { }; } +/// Which path produced one [`Event::AssistantReply`]: a user-facing chat +/// turn ([`Chat`](Self::Chat)) or a programmatic inference round +/// ([`Infer`](Self::Infer)). +/// +/// The default is `chat`, so an older log written before the field existed +/// reads back as a chat reply. The enum is `#[non_exhaustive]`, so a host +/// matches the two known origins and keeps a wildcard for a future one. +#[derive(Debug, Clone, Copy, Default, PartialEq, Eq, Serialize, Deserialize)] +#[serde(rename_all = "snake_case")] +#[non_exhaustive] +pub enum ReplyOrigin { + /// A user-facing chat turn: the reply belongs in the conversation. + #[default] + Chat, + /// A programmatic inference round (`models.infer`): the reply is a + /// model result the host may treat apart from the conversation. + Infer, +} + events! { /// One thing that happened during a run. /// @@ -324,7 +343,8 @@ events! { /// The thinking text: untrusted model output. text: String, }, - /// One completed assistant reply. + /// One completed assistant reply: a model round's text reply, + /// carrying the [`ReplyOrigin`] of the round that produced it. AssistantReply { /// The model-turn counter the reply was produced under. turn: u32, @@ -336,6 +356,11 @@ events! { model: String, /// Everything the call measured, when anything reported. metrics: Option, + /// The provenance a host inspects to distinguish an inference + /// round (`infer`) from a user-facing chat turn (`chat`). + /// Defaults to `chat` when an older log carries no `origin`. + #[serde(default)] + origin: ReplyOrigin, }, /// One batch of tool calls the model requested, unexecuted. AssistantToolCalls { diff --git a/crates/promptforge/lua/src/protocol/tests/answer.rs b/crates/promptforge/lua/src/protocol/tests/answer.rs index c6dd839b..9869fe6b 100644 --- a/crates/promptforge/lua/src/protocol/tests/answer.rs +++ b/crates/promptforge/lua/src/protocol/tests/answer.rs @@ -79,6 +79,7 @@ fn a_task_events_answer_resumes_event_tables_with_absent_fields_nil() { finish_reason: None, model: "m".to_owned(), metrics: None, + origin: promptforge_api_types::event::ReplyOrigin::Chat, }, ]; let (envelope, retained) = Answer::::TaskEvents(Ok(events)) diff --git a/crates/workshop/protocol/tests/it/fixture.rs b/crates/workshop/protocol/tests/it/fixture.rs index 62212b0a..a63f7368 100644 --- a/crates/workshop/protocol/tests/it/fixture.rs +++ b/crates/workshop/protocol/tests/it/fixture.rs @@ -54,6 +54,7 @@ fn stamped_fixture_event() -> promptforge_api_types::event::Event { text: "hello".to_owned(), finish_reason: Some("stop".to_owned()), model: "llama-3".to_owned(), + origin: promptforge_api_types::event::ReplyOrigin::Chat, metrics: Some(CallMetrics { usage: Some(Usage { prompt_tokens: 7, diff --git a/crates/workshop/server/src/agents/status-tests.rs b/crates/workshop/server/src/agents/status-tests.rs index dbd2dc6a..70b09d3d 100644 --- a/crates/workshop/server/src/agents/status-tests.rs +++ b/crates/workshop/server/src/agents/status-tests.rs @@ -46,6 +46,7 @@ fn reply_event() -> Event { finish_reason: None, model: "m".to_owned(), metrics: None, + origin: promptforge_api_types::event::ReplyOrigin::Chat, } } From 0d62bb5f232f983a8e857064b453e18efba4eb80 Mon Sep 17 00:00:00 2001 From: Vinnie Falco Date: Tue, 22 Sep 2026 12:02:46 -0700 Subject: [PATCH 05/19] =?UTF-8?q?=EF=BB=BFDocument=20the=20model=20reply?= =?UTF-8?q?=20origin?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The agent guide and the papergate migration record described a separate inference reply kind that no longer exists. Record the reply origin instead, and mark the emit-infer-reply plan as superseded by the merged design. - guide/src/agent/05-the-event-log.md documents origin (chat or infer) on a model round's reply; the generated guide is regenerated. - vibe/papergate-harness-migration.md reads assistant_reply with origin infer. - vibe/2026-09-22-2-emit-infer-reply.md aligns to the merged design and marks its distinct-kind step record as superseded. Plan: none --- guide/promptforge-agent-guide.md | 2 +- guide/src/agent/05-the-event-log.md | 2 +- vibe/2026-09-22-2-emit-infer-reply.md | 229 ++++++++++++++++++++++++++ vibe/papergate-harness-migration.md | 4 +- 4 files changed, 233 insertions(+), 4 deletions(-) create mode 100644 vibe/2026-09-22-2-emit-infer-reply.md diff --git a/guide/promptforge-agent-guide.md b/guide/promptforge-agent-guide.md index 52450f93..66de25d9 100644 --- a/guide/promptforge-agent-guide.md +++ b/guide/promptforge-agent-guide.md @@ -311,7 +311,7 @@ The log grows only when the run resumes from a host call, never in the middle of Every entry has `kind`, `execution`, `section`, and `provenance`. The `kind` reads as a pinned snake_case label, such as `assistant_reply`, `tool_result`, `user_input`, `lua`, or `task_started`, and the rest of the table is that kind's own fields. -A model round leaves `assistant_reply` (with `turn`, `text`, `model`, `finish_reason`, and `metrics`) or `assistant_tool_calls` (with the requested `calls`); a block of reasoning leaves `thinking`. Every dispatched tool call leaves `tool_call_succeeded` or `tool_call_failed`, and a call the model issued also leaves `tool_result` with `turn`, `tool_call_id`, `alias`, `content`, and `trusted`. Operator text arrives as `user_input`, and your own `log(...)` checkpoints as `lua` with `message`. Background work leaves `task_started` with its spawn seeds, then one of `task_succeeded`, `task_failed`, `task_cancelled`, or `task_abandoned`. +A model round leaves `assistant_reply` (with `turn`, `text`, `model`, `finish_reason`, `metrics`, and an `origin`) or `assistant_tool_calls` (with the requested `calls`); a reply's `origin` is `chat` for a user-facing turn and `infer` for a tool-free inference round, so a host that inspects `origin` can tell the two apart. A block of reasoning leaves `thinking`. Every dispatched tool call leaves `tool_call_succeeded` or `tool_call_failed`, and a call the model issued also leaves `tool_result` with `turn`, `tool_call_id`, `alias`, `content`, and `trusted`. Operator text arrives as `user_input`, and your own `log(...)` checkpoints as `lua` with `message`. Background work leaves `task_started` with its spawn seeds, then one of `task_succeeded`, `task_failed`, `task_cancelled`, or `task_abandoned`. An absent optional field, such as a reply with no `finish_reason`, reads as nil, so test presence with a plain truth test. diff --git a/guide/src/agent/05-the-event-log.md b/guide/src/agent/05-the-event-log.md index 434edaa8..393237d6 100644 --- a/guide/src/agent/05-the-event-log.md +++ b/guide/src/agent/05-the-event-log.md @@ -36,7 +36,7 @@ The log grows only when the run resumes from a host call, never in the middle of Every entry has `kind`, `execution`, `section`, and `provenance`. The `kind` reads as a pinned snake_case label, such as `assistant_reply`, `tool_result`, `user_input`, `lua`, or `task_started`, and the rest of the table is that kind's own fields. -A model round leaves `assistant_reply` (with `turn`, `text`, `model`, `finish_reason`, and `metrics`) or `assistant_tool_calls` (with the requested `calls`); a block of reasoning leaves `thinking`. Every dispatched tool call leaves `tool_call_succeeded` or `tool_call_failed`, and a call the model issued also leaves `tool_result` with `turn`, `tool_call_id`, `alias`, `content`, and `trusted`. Operator text arrives as `user_input`, and your own `log(...)` checkpoints as `lua` with `message`. Background work leaves `task_started` with its spawn seeds, then one of `task_succeeded`, `task_failed`, `task_cancelled`, or `task_abandoned`. +A model round leaves `assistant_reply` (with `turn`, `text`, `model`, `finish_reason`, `metrics`, and an `origin`) or `assistant_tool_calls` (with the requested `calls`); a reply's `origin` is `chat` for a user-facing turn and `infer` for a tool-free inference round, so a host that inspects `origin` can tell the two apart. A block of reasoning leaves `thinking`. Every dispatched tool call leaves `tool_call_succeeded` or `tool_call_failed`, and a call the model issued also leaves `tool_result` with `turn`, `tool_call_id`, `alias`, `content`, and `trusted`. Operator text arrives as `user_input`, and your own `log(...)` checkpoints as `lua` with `message`. Background work leaves `task_started` with its spawn seeds, then one of `task_succeeded`, `task_failed`, `task_cancelled`, or `task_abandoned`. An absent optional field, such as a reply with no `finish_reason`, reads as nil, so test presence with a plain truth test. diff --git a/vibe/2026-09-22-2-emit-infer-reply.md b/vibe/2026-09-22-2-emit-infer-reply.md new file mode 100644 index 00000000..b0d11322 --- /dev/null +++ b/vibe/2026-09-22-2-emit-infer-reply.md @@ -0,0 +1,229 @@ +--- +name: Emit an infer-origin assistant_reply +overview: Report tool-less `models.infer` turns as an `assistant_reply` carrying `origin: ReplyOrigin` = `infer`, emitted by `accept_infer_completion` with full session-lifecycle parity (turn settle, reply stamp), so hosts see every model round through the one reply kind, scoped to `harness-api` session-event clients and leaving the workshop agent protocol untouched. +todos: + - id: api-types-variant + content: Add ReplyOrigin and the AssistantReply origin field, give Emitter::assistant_reply an origin parameter, and cover the round-trip in promptforge-api-types + status: pending + - id: hoist-metrics + content: Hoist call_metrics to execute/support.rs as pub(crate) and re-point chat.rs + status: pending + - id: emit-infer-reply + content: Capture model/metrics/thinking before the result move, then emit thinking and an infer-origin assistant_reply from accept_infer_completion; update its doc comment + status: pending + - id: observer-forwarding + content: Add origin to on_assistant_reply on the test_support observer (dropping on_infer_reply) and forward it + status: pending + - id: session-parity + content: "Give infer-origin replies session parity: one AssistantReply arm covers both origins in settle_current_turn and reply_stamp in session.rs" + status: pending + - id: tests + content: Add api-runtime (execute/tests) and sessions (session-tests.rs + tests/it/session.rs) tests, including the reply-index sequence and a chat-origin assertion + status: pending + - id: verify + content: Run crate test suites and a local tool-less infer reproduction + status: pending +isProject: false +--- + +# Emit an infer-origin assistant_reply for tool-less model inference + + + +## Product Requirements + +A Lua `models.infer(prose)` call in a section that advertises no tools returns the model's text to Lua, but the engine emits only `model_turn_completed` for that turn - no content event - so a host following session events through `harness-api` never sees the reply text, and no run outcome value is exposed either. Observed at master `b64c1c9d`: the event sequence for a tool-less infer section runs `model_turn_completed` straight to `lua_chunk_succeeded` with no `assistant_reply` and no `thinking`, while the text demonstrably arrives in Lua. The fix reports that round as an `assistant_reply` carrying `origin = infer`, so session-event clients see every model reply through the one reply kind, whether or not tools were advertised. + +- Problem and users: the infer path emits no content event for a text result, so `harness-api` session-event clients (the papergate port being the driving case - it reads the run's last reply event as its report) cannot obtain a tool-less infer's text; affected users are prompt authors and hosts running agent sessions whose final step is a tool-less `models.infer`. +- Goals: every tool-less infer text turn emits an `assistant_reply` with `origin = infer` carrying the text, finish reason, model, and metrics; the event stream distinguishes text / empty / tool-batch outcomes for tool-less turns; infer reasoning emits `thinking` in parity with the chat path; session lifecycle treats an infer-origin `assistant_reply` exactly like a chat-origin one. +- Non-goals: no workshop agent-protocol mapping; no `RunOutcome`/`final_text` exposure through `harness-api` (tracked separately); no reader-tolerance shim for pre-change log readers. +- Success criteria: a host subscribed to `harness-api` session events sees an infer-origin `assistant_reply` between `model_turn_completed` and `lua_chunk_succeeded` for a tool-less infer turn; the success consumer is the session sink in `crates/harness/sessions/src/session.rs`, not "hosts" generally - because the origin is new information, a consumer that ignores it still gets the reply, and one that inspects it can separate the inference round. +- Constraints: the existing `assistant_reply` event keeps its meaning and gains a typed `origin`; `Event` stays `#[serde(tag = "kind", rename_all = "snake_case")]` and `#[non_exhaustive]`; the reply text remains documented as untrusted model output. +- Open questions: None + +## Functional Specification + +The engine's infer path reports a completed text round with the same event richness as a chat prose round, through the shared `assistant_reply` kind whose `origin` field marks the reply as a programmatic inference result rather than user-facing chat. The session sink folds both origins into its existing lifecycle and reply-stamping rules. Hosts that ignore `origin` see no change in the events they already read. + +```mermaid +flowchart LR + Infer[accept_infer_*] -->|assistant_reply origin=infer| Sink[session sink] + Chat[chat text_reply] -->|assistant_reply origin=chat| Sink + Sink --> Host[harness-api host] +``` + +- Actors and workflows: `accept_infer_completion` (`crates/promptforge-api-runtime/src/execute/tools.rs`) calls the shared `report_model_turn`; the `Emitter` (`crates/promptforge-api-types/src/emitter.rs`) carries; the session sink (`crates/harness/sessions/src/session.rs`) settles, stamps, and forwards to the subscribed host. +- Inputs and outputs: input is the infer round's `Completion`; output is an `assistant_reply` event with `origin = infer` and payload `{turn, text, finish_reason, model, metrics, origin}` - the same fields as a chat reply plus the provenance - and a `thinking` event when the completion carries non-empty reasoning content. +- States and validation: event sequence for a tool-less infer turn is `model_turn_completed`, `thinking` (when present), `model_turn_truncated` (on a `length` finish), an infer-origin `assistant_reply`, mirroring the chat path's ordering in `served()` + `text_reply`; `reply_stamp` stamps an `AssistantReply` of either origin with the current round and advances it, applied identically live and on replay. +- Errors and recovery: a tool-call outcome on a tool-less infer remains the backend-protocol-violation error it is today; an unrecognized outcome likewise; no new failure modes. +- Security and privacy behavior: the `text` field is untrusted model output, documented as such on `AssistantReply`; no credential-bearing or raw-body data is added to any event. +- Acceptance criteria: the report's reproduction prompt (a section whose only model call is `return models.infer(prose)`) produces a session event stream containing an infer-origin `assistant_reply` with the reply text; no chat-origin reply is emitted for the infer turn; existing chat-path sessions are unchanged except that mixed infer-then-chat sessions see reply indices shift (accepted, pinned by test). + + + + +## Technical Design + +`Event` is an internally-tagged, `#[non_exhaustive]` serde enum generated by the `events!` macro (`crates/promptforge-api-types/src/event.rs`, lines 85-88). The reply distinction is carried by a typed field on the existing reply variant, not by a second kind: `ReplyOrigin { Chat, Infer }` derives `Default` with `Chat` as its default, serializes `#[serde(rename_all = "snake_case")]`, and is attached to `AssistantReply` as `#[serde(default)] origin`. A new reader parses an old log with `origin` defaulting to `chat`; an old reader ignores the unknown `origin` field on a new log. No kind registry or golden enumerates event kinds, so no list beyond the macro invocation needs updating. + +- Architecture: one `assistant_reply` event carries the provenance of the round that produced it (`origin`: `chat` or `infer`), superseding the separate `InferReply` variant. `AssistantReply` keeps its meaning as a model round's text reply and gains a typed provenance; session lifecycle treats both origins as one model-round content kind (turn settle, reply stamp), so an infer round is a first-class model round in session bookkeeping. +- Modules and interfaces: `crates/promptforge-api-types/src/event.rs` adds `ReplyOrigin { Chat, Infer }` and an `origin` field on `AssistantReply { turn, text, finish_reason, model, metrics, origin }`, field docs mirroring the rest including the untrusted-output note; `crates/promptforge-api-types/src/emitter.rs` gives `assistant_reply` an `origin` parameter and drops `infer_reply`; `crates/promptforge-api-runtime/src/execute/support.rs`'s shared `report_model_turn` takes a `ReplyOrigin` in place of the deleted `ReplyKind`, so the chat arm passes `Chat` and `accept_infer_completion` passes `Infer` with the same event ordering and payloads. +- File and public API changes: the three files above plus `crates/promptforge-api-runtime/src/execute/scheduler/chat.rs` and `crates/promptforge-api-runtime/src/execute/tools.rs` (origin at the two emit sites), `crates/promptforge-api-runtime/src/test_support/recording.rs` (drops `on_infer_reply`; `on_assistant_reply` gains the origin), `crates/promptforge-api-runtime/src/test_support/recording-forward.rs`, `crates/harness/sessions/src/session.rs` (one `AssistantReply` arm covers both origins in the settle and reply-stamp rules), and test files. The public API change is the `ReplyOrigin` enum and the extra `origin` parameter; `InferReply` and `Emitter::infer_reply` are removed. +- Data, persistence, failure, security, and privacy constraints: `origin` serializes snake_case and is `#[serde(default)]`, so old logs parse as `chat` and new logs carry the field; the reply text stays documented as untrusted model output; no new failure modes and no credential-bearing or raw-body data is added to any event. + + + + +## Testing Plan + +Unit tests pin the event's wire round-trip for both origins and the reply-stamp rule; the existing api-runtime infer suites gain sequence assertions; a sessions integration test pins the mixed-session reply-index sequence. The manual reproduction substitutes for the external papergate run, since this checkout has no `crates/papergate`. + +- Unit: extend the event round-trip test in `crates/promptforge-api-types/src/event-tests.rs` with an `AssistantReply` case for `origin = infer` (and assert the `Chat` default); add session-sink unit coverage for `reply_stamp` under an `AssistantReply` of either origin (there is currently no test module in `session.rs`; add an inline `#[cfg(test)] mod tests` or a sibling `session-tests.rs` wired by `#[path]`), asserting an infer-origin reply stamps the current round and advances it and that `Thinking` still stamps without advancing. +- Integration and end-to-end: extend the infer tests in `crates/promptforge-api-runtime/src/execute/tests/debug_and_counts.rs`, which already drive tool-less `models.infer` rounds and inspect observed events, to assert the sequence `model_turn_completed` -> (`thinking` when present) -> an infer-origin `assistant_reply` with the text and that no chat-origin reply is emitted; add an infer test whose completion carries `reasoning_content` to cover the `thinking` parity path; extend `crates/harness/sessions/tests/it/session.rs` with a mixed infer-then-chat session asserting the reply-index sequence and confirming `settle_current_turn` fires on an infer-origin `AssistantReply` (pattern at `lifecycle.rs:130`); pin the chat emit site by asserting a chat round reports `origin = Chat` in `crates/promptforge-api-runtime/src/execute/tests/chat_arm.rs`. +- Regression, security, and performance: grep the sessions tests for any pre-existing reply-index assertion and renumber to the new sequence; confirm no wire snapshot or golden enumerates event kinds (step 1 of execution established none exists; assert it in the test pass); confirm the `text` field docs carry the untrusted-output note. +- Exit criteria: `cargo test -p promptforge-api-types -p promptforge-api-runtime -p harness-sessions` passes, then the wider harness suites; manual run of a prompt whose section calls `models.infer(prose)` tool-lessly through `harness-api` shows an infer-origin `assistant_reply` between `model_turn_completed` and `lua_chunk_succeeded` in the session event stream. + + + + +## Decision Record + +- Decisions: + - Merge the infer reply into `AssistantReply` as a typed `origin: ReplyOrigin { Chat, Infer }`, superseding the earlier separate-`InferReply`-variant decision: one reply event keeps `AssistantReply`'s meaning intact while giving hosts provenance to distinguish an inference round from a chat turn; the field is `#[serde(default)]` with a `Chat` default. + - Full session-lifecycle parity across origins (settle current turn, reply stamp advances the round): both origins are model-round content, and `reply_stamp` applies identically live and on replay, so reply indices in mixed infer-then-chat sessions move together (accepted, pinned by test). + - Carry the origin through the recording observer rather than adding a second hook: `on_assistant_reply` gains an `origin` parameter and `on_infer_reply` is dropped, so runtime unit tests observe through the one reply hook; session tests read the raw event's `origin`. Chosen as the smaller change because the runtime tests observe through the `Observer` seam and cannot read raw events. + - Defer the workshop agent-protocol mapping: workshop matches are wildcard/if-let, so nothing breaks; no workshop consumer requests infer text. User's words: selected "Defer workshop (Recommended)". + - Emit `thinking` for non-empty infer reasoning content: parity with the chat path's `served()`; the bug report noted its absence from the infer sequence. + - Capture model, metrics, thinking, and finish reason before the `match completion.result` partial move (E0382), mirroring `served()`'s documented ordering. + - Keep the per-origin payloads identical: the shared `report_model_turn` fires the same sequence and emits exactly one `assistant_reply` carrying the caller's `origin`. +- Rejected alternatives: + - A separate `InferReply` event variant: duplicates the reply payload and asks every host to learn a second kind; superseded by the merged `origin` field on `AssistantReply`. + - Reusing `section` or `provenance` to mark the producing path: stringly-typed abuse of fields that already have jobs (replay key, reporting scope). + - Exposing `RunOutcome.final_text` through `harness-api` as the only fix: complementary API addition that does not repair event-stream consistency (a host still could not distinguish text / empty / tool-batch for a tool-less turn). Deferred, not rejected outright. +- Assumptions, risks, and notes: + - Note: `origin` is `#[serde(default)]` = `chat`, so an old reader ignores the unknown field on a new log and a new reader parses an old log; logs are readable in both directions. + - Note: no kind registry or golden enumerates event kinds; the `lifecycle` module holds payload-free boundary events only, so the macro invocation is the only list to update. + - Note: `call_metrics` hoists to `execute/support.rs`, the module `tools.rs` already imports `advance_turn` from. + - Note: the `Observer` trait's default-bodied methods require `#[expect(unused_variables, ...)]` under the workspace's clippy deny-warnings configuration. + +### Deferred and Out of Scope + +- Deferred: workshop agent-protocol mapping (`AgentEventKind::InferReply` in `crates/workshop/protocol/src/agent.rs`, status handling in `crates/workshop/server/src/agents/status.rs`); revisit when a workshop consumer needs infer text. +- Deferred: exposing `RunOutcome` (`final_text`) through `harness-api`; revisit as a separate API addition. +- Out of scope: changing the meaning of `AssistantReply`'s existing fields; changing chat-path event ordering; any gateway or wire-protocol change. + + + + +## Project Survey + +- Status: complete +- Build command: `cargo build --locked -p gateway` (gateway is the default member; `cargo build --locked -p workshop` for the desktop app, or `cargo workshop` for the staged one-command build; run `npm ci` in `crates/workshop/ui` and `crates/gateway/config-ui/ui` once first) +- Focused test command pattern: `cargo nextest run --locked -p ` (alternately `cargo test --locked -p --test it `) +- Component test command pattern: `cargo nextest run --locked -p ` (add `--all-features` where the crate gates features); workshop: `cargo nextest run --locked -p workshop -p workshop-server -p workshop-server-api` +- Full-suite test command: `cargo nextest run --locked --workspace --exclude workshop --exclude workshop-server --exclude workshop-server-api --all-features`, then doctests via `cargo test --workspace --exclude workshop --exclude workshop-server --exclude workshop-server-api --all-features --doc` +- Linter command: `cargo clippy --workspace --exclude workshop --exclude workshop-server --exclude workshop-server-api --all-targets --all-features -- -D warnings` (workshop: `cargo clippy -p workshop -p workshop-server -p workshop-server-api --all-targets -- -D warnings`); never run a standalone `cargo check --workspace` beside it +- Formatter check command: `cargo fmt --all --check` +- Docs command: `RUSTDOCFLAGS="-D warnings" cargo doc --workspace --no-deps --all-features --exclude workshop --exclude workshop-server --exclude workshop-server-api`; user guide: `mdbook build guide`; structural harness: `cargo test -p build-xtask` +- Test placement and naming conventions: Unit tests are inline `#[cfg(test)] mod tests` in source files; integration tests are a single `it` target rooted at `crates//tests/it/main.rs`, with some crates using `tests/suite/` or standalone `tests/.rs` targets; fixtures live in `tests/fixtures/` and shared helpers in `tests/common/`. Test functions are long snake_case sentences (e.g. `a_direct_launch_recovers_the_lease_from_a_terminated_owner`). Ordinary `cargo test` stays fully offline. UI packages under `crates/*/ui` test with `npm test`; Node scripts under `tools/` have sibling `*.test.mjs` files; benchmarks use criterion (dev-only). +- Directory map: The root holds the Cargo workspace manifests (`Cargo.toml`, `clippy.toml`, `rustfmt.toml`, `deny.toml`, `rust-toolchain.toml`, `dist-workspace.toml`), `README.md`/`AGENTS.md`, and the `.github/workflows/`, `.cargo/`, `.config/`, `.githooks/`, `crates/`, `guide/`, `images/`, `local/`, `prompts/`, `tools/`, and `vibe/` trees. `crates/` is the public and shared layer; the family containers `crates/promptforge/`, `crates/gateway/`, `crates/workshop/`, and `crates/harness/` are private and hold each product's crates; `crates/shared-ui` is a TypeScript+CSS package, not a Rust crate. +- Component boundaries: The architecture components are the executor, harness, gateway, CLI, workshop UI, store, VFS layer, Lua VM boundary, and shared substrate. Dependencies flow one way (`shell -> features -> services -> vocabulary`): the executor is sans-I/O and depends on the store, the Lua VM boundary, and the shared substrate; the harness hosts it; the gateway is independent; the CLI and workshop UI sit on top. Each family's public surface is a single root crate (`promptforge-api-runtime`/`promptforge-api-types`, `gateway-api-types`/`gateway-api-discovery`, `harness-api`), and no outside crate may depend into a family container. +- Conventions summary: Rust 2024 edition, BSL-1.0; workspace lints deny `unsafe_code`, `unwrap_used`, `expect_used`, and `all`/`pedantic` clippy; every workshop-*/harness-* lib.rs carries a `## Invariants` marker and its Rust files stay under 500 lines; source directories are flat until a sibling group reaches three files; behavior changes ship tests in the same change; comments explain non-obvious constraints and cite upstream issue URLs for workarounds; SPA CSS lives beside its TypeScript and uses `--ws-*` tokens; the SPA never touches `localStorage`, persisting UI state through the server; structural enforcement lives in `cargo test -p build-xtask`. + + + + +## Execution Instructions + +Components in dependency order; each is a shippable crate-level package: + +1. `promptforge-api-types` - the event vocabulary. Placed first because the `origin` field on `AssistantReply` gates everything else: nothing compiles against it until it exists. No new dependency. +2. `promptforge-api-runtime` - the producer. Depends on the field; internally sequential because the `call_metrics` hoist must land before the emission that calls it. +3. `harness-sessions` - the consumer that settles and stamps the event. Depends on the field; internally sequential because the lifecycle arms must land before the sequence test that pins them. +4. `verification` - the exit-criteria run. Depends on all three prior components; single step. + +> **Historical note (superseded plan).** Steps 1-6 below are the build record of the earlier, superseded design that introduced a distinct `InferReply` variant and `Emitter::infer_reply`. The shipped implementation instead merged that variant into `AssistantReply` as `origin: ReplyOrigin { Chat, Infer }` driven by the shared `report_model_turn`; the steps below are retained only as history and are **not** current instructions. Consult the Technical Design and Decision Record above for the delivered shape. + + + +### Step 1 (superseded distinct-kind record): Add the InferReply event variant and Emitter::infer_reply [completed] + +- Component: `promptforge-api-types` + +Add the `InferReply { turn, text, finish_reason, model, metrics }` variant to the `events!` invocation in `crates/promptforge-api-types/src/event.rs` next to `AssistantReply` (line 328), with field docs mirroring that variant, including the untrusted-model-output note on `text`. Add `Emitter::infer_reply` in `crates/promptforge-api-types/src/emitter.rs`, mirroring `assistant_reply` (line 272). + +Tests: extend the round-trip test in `crates/promptforge-api-types/src/event-tests.rs` (pattern at line 66) with an `InferReply` case. + +Constraints: `Event` stays `#[serde(tag = "kind", rename_all = "snake_case")]` and `#[non_exhaustive]`; the existing `assistant_reply` meaning is untouched. + + + + + +### Step 2 (superseded distinct-kind record): Hoist call_metrics into execute/support.rs [completed] + +- Component: `promptforge-api-runtime` + +Move `call_metrics` from `crates/promptforge-api-runtime/src/execute/scheduler/chat.rs` (line 59) to `crates/promptforge-api-runtime/src/execute/support.rs` as `pub(crate)`, and re-point `chat.rs` to the new path. The sibling module `tools.rs` already imports `advance_turn` from `support.rs`, so the hoist adds no new dependency edge. + +Tests: existing chat-path and scheduler suites pass unchanged (`cargo nextest run --locked -p promptforge-api-runtime`), proving the hoist is behavior-preserving. + +Sequencing: this must land before Step 3, which calls `support::call_metrics`. + + + + + +### Step 3 (superseded distinct-kind record): Emit thinking and infer_reply from accept_infer_completion [completed] + +- Component: `promptforge-api-runtime` + +In `accept_infer_completion` (`crates/promptforge-api-runtime/src/execute/tools.rs`, lines 26-64): capture metrics, model, `reasoning_content`, and finish reason before the `match completion.result` partial move (E0382), mirroring `served()` (chat.rs lines 319-327); emit `thinking` after `MODEL_TURN_COMPLETED` when reasoning is non-empty; emit `infer_reply` in the `CompletionResult::Text` arm after the `model_turn_truncated` report and before `Ok(text)`; update the doc comment to name `infer_reply`. + +Add `on_infer_reply` to the `Observer` trait in `crates/promptforge-api-runtime/src/test_support/recording.rs` with the sibling `#[expect(unused_variables, reason = ...)]` body, and forward the variant in `crates/promptforge-api-runtime/src/test_support/recording-forward.rs` (arms at lines 103 and 253). + +Tests: extend `crates/promptforge-api-runtime/src/execute/tests/debug_and_counts.rs` to assert the sequence `model_turn_completed` -> (`thinking` when present) -> `infer_reply` carrying the text and that no `assistant_reply` is emitted; add a case whose completion carries `reasoning_content` for the thinking parity path. + +Sequencing: observer forwarding and emission share one observation surface, so one test set covers the step completely. + + + + + +### Step 4 (superseded distinct-kind record): Give InferReply session lifecycle parity [completed] + +- Component: `harness-sessions` + +In `crates/harness/sessions/src/session.rs`, add `Event::InferReply { .. }` to the settle arm (line 444) and to the stamped-and-advancing arm of `reply_stamp` (lines 470-474), so an infer reply settles the current turn and advances the round exactly like `AssistantReply`, identically live and on replay. + +Tests: add an inline `#[cfg(test)] mod tests` (or a `session-tests.rs` sibling wired by `#[path]`) covering `reply_stamp` under `InferReply` (stamps the current round and advances) and under `Thinking` (stamps without advancing). + +Sequencing: these arms must land before the integration test in Step 5 that pins their sequence. + + + + + +### Step 5 (superseded distinct-kind record): Pin the mixed-session reply-index sequence [completed] + +- Component: `harness-sessions` + +Extend `crates/harness/sessions/tests/it/session.rs` with a mixed infer-then-chat session that asserts the reply-index sequence and confirms `settle_current_turn` fires on `InferReply` (pattern at `lifecycle.rs:130`). Grep the sessions tests for any pre-existing reply-index assertion and renumber it to the new sequence. + +Tests: the new integration test, plus a check that no wire snapshot or golden enumerates event kinds. + + + + + +### Step 6 (superseded distinct-kind record): Run the exit criteria and the manual reproduction [completed] + +- Component: `verification` + +Run `cargo test -p promptforge-api-types -p promptforge-api-runtime -p harness-sessions`, then the wider harness suites. Confirm no wire snapshot or golden enumerates event kinds and that the `text` field docs carry the untrusted-output note. Run the manual reproduction: a tool-less `return models.infer(prose)` section through `harness-api` shows `infer_reply` between `model_turn_completed` and `lua_chunk_succeeded` in the session event stream. + +Tests: exit-criteria suites green; manual reproduction observed. + + + + diff --git a/vibe/papergate-harness-migration.md b/vibe/papergate-harness-migration.md index be7c8630..80119b35 100644 --- a/vibe/papergate-harness-migration.md +++ b/vibe/papergate-harness-migration.md @@ -54,7 +54,7 @@ Through `harness-api` there is no store access in either direction. A session's Two ways to close the gap, for Papergate's own plan to choose: -1. Change the prompt, not the door. Deliver the paper as the run's argument (`LaunchRequest::args`) and have `papergate.md` read `args` instead of `store.read("paper.md")`, keeping `store.write("paper.md", args)` as its first statement if the `read_numbered` line ranges in `### Evaluate` are to stay as they are. Deliver the report as model text: the `## Analyze` section's `models.infer(prose)` already produces the report, and that call leaves an `assistant_reply` event with the report in `text` under `section == "Analyze"`. Papergate takes the last such event from the transcript. The `input:` and `output:` frontmatter declarations become documentation only. Cost: a 32k-context paper travels as one argument string, and the report is read from an event rather than a declared output. Recommended: it needs no change to the promptforge repository (confidence: medium; depends on Papergate accepting an event as the report's channel). +1. Change the prompt, not the door. Deliver the paper as the run's argument (`LaunchRequest::args`) and have `papergate.md` read `args` instead of `store.read("paper.md")`, keeping `store.write("paper.md", args)` as its first statement if the `read_numbered` line ranges in `### Evaluate` are to stay as they are. Deliver the report as model text: the `## Analyze` section's `models.infer(prose)` already produces the report, and that call leaves an `assistant_reply` event with `origin: infer` carrying the report in `text` under `section == "Analyze"`. Papergate takes the last such event from the transcript. The `input:` and `output:` frontmatter declarations become documentation only. Cost: a 32k-context paper travels as one argument string, and the report is read from an event rather than a declared output. Recommended: it needs no change to the promptforge repository (confidence: medium; depends on Papergate accepting an event as the report's channel). 2. Extend the door. Give `LaunchRequest` an optional host root (a directory mounted into the run's VFS beside the fresh store, or a set of seed files written into the store), and give `Session` a way to read the run's final text or a store file once `Closed`. This is a promptforge change with its own plan; the `HostSnapshot::workspace_roots` field already exists and is the natural carrier, but today it feeds only the `ui()` snapshot and mounts nothing. ## The shape of the new run @@ -76,7 +76,7 @@ loop { } } let transcript = session.transcript(0).await?; -// option 1: the report is the last `assistant_reply` event under section "Analyze" +// option 1: the report is the last `assistant_reply` event with `origin: infer` under section "Analyze" ``` `agents_path` holds `papergate.md` (the embedded default or the `--prompt` file), and `state_dir` receives the harness's `runs.db`; both may be temporary directories removed after the run, as the store directory is today. The run log is the durable record the old stderr observer approximated; keep `state_dir` when the transcript is worth retaining. From 754e294c82e8a4f23bb6b1aa35913036e115415e Mon Sep 17 00:00:00 2001 From: Vinnie Falco Date: Tue, 22 Sep 2026 14:47:31 -0700 Subject: [PATCH 06/19] Log upstream client errors at info; move vibe plans Raise the level of the upstream 4xx log line in `OpenAiUpstream` from `tracing::debug!` to `tracing::info!` so client errors appear in default log output. Also move the 36 September plan files from `vibe/` into a `vibe/2026-09/` subdirectory with no content changes. - The 5xx branch stays at `tracing::warn!`; only the client error branch changes level. - No tests touch the log level. --- crates/gateway/protocol/src/upstream.rs | 2 +- vibe/{ => 2026-09}/2026-09-10-1-unified-prompt-model.md | 0 vibe/{ => 2026-09}/2026-09-10-2-debt-fixes.md | 0 vibe/{ => 2026-09}/2026-09-11-1-rulebook-debt-tiers.md | 0 vibe/{ => 2026-09}/2026-09-11-2-unify-toolchain.md | 0 vibe/{ => 2026-09}/2026-09-11-3-vfs-foundation.md | 0 vibe/{ => 2026-09}/2026-09-12-1-test-namespace-vfs-debt.md | 0 vibe/{ => 2026-09}/2026-09-12-2-dependency-rules-vfs-hook.md | 0 .../{ => 2026-09}/2026-09-12-3-workshop-server-decomposition.md | 0 vibe/{ => 2026-09}/2026-09-12-4-workshop-debt-removal.md | 0 vibe/{ => 2026-09}/2026-09-12-5-one-door-promptforge-api.md | 0 vibe/{ => 2026-09}/2026-09-13-1-capabilities-global-naming.md | 0 .../2026-09-14-1-debt-removal-capabilities-follow-ups.md | 0 vibe/{ => 2026-09}/2026-09-14-2-provider-model-sheets.md | 0 vibe/{ => 2026-09}/2026-09-14-3-fix-previous-sheet-swallow.md | 0 vibe/{ => 2026-09}/2026-09-14-4-provider-expansion-local-run.md | 0 vibe/{ => 2026-09}/2026-09-14-5-models-aggregation-workflow.md | 0 vibe/{ => 2026-09}/2026-09-14-6-model-taxonomy-cloud-ui.md | 0 vibe/{ => 2026-09}/2026-09-15-1-cloud-sheet-debt-removal.md | 0 vibe/{ => 2026-09}/2026-09-15-2-profiles-gate-local.md | 0 vibe/{ => 2026-09}/2026-09-15-3-profiles-debt-removal.md | 0 vibe/{ => 2026-09}/2026-09-15-4-flat-sources.md | 0 vibe/{ => 2026-09}/2026-09-15-5-workshop-menu-overhaul.md | 0 vibe/{ => 2026-09}/2026-09-16-1-turso-workspace-files.md | 0 vibe/{ => 2026-09}/2026-09-16-2-workshop-ui-state.md | 0 vibe/{ => 2026-09}/2026-09-16-3-workspace-debt-removal.md | 0 vibe/{ => 2026-09}/2026-09-17-1-promptforge-family-reorg.md | 0 vibe/{ => 2026-09}/2026-09-17-2-gateway-family-reorg.md | 0 vibe/{ => 2026-09}/2026-09-17-3-gateway-app-decomp.md | 0 vibe/{ => 2026-09}/2026-09-17-4-ci-repair-state-accessors.md | 0 vibe/{ => 2026-09}/2026-09-17-5-workshop-family-reorg.md | 0 vibe/{ => 2026-09}/2026-09-17-6-minimal-run-window.md | 0 vibe/{ => 2026-09}/2026-09-18-1-single-rustls-backend.md | 0 vibe/{ => 2026-09}/2026-09-18-2-remove-tool-picker.md | 0 vibe/{ => 2026-09}/2026-09-18-3-empty-zone-groups.md | 0 vibe/{ => 2026-09}/2026-09-18-4-sans-io-engine-harness.md | 0 vibe/{ => 2026-09}/2026-09-19-1-chatbox-extraction.md | 0 37 files changed, 1 insertion(+), 1 deletion(-) rename vibe/{ => 2026-09}/2026-09-10-1-unified-prompt-model.md (100%) rename vibe/{ => 2026-09}/2026-09-10-2-debt-fixes.md (100%) rename vibe/{ => 2026-09}/2026-09-11-1-rulebook-debt-tiers.md (100%) rename vibe/{ => 2026-09}/2026-09-11-2-unify-toolchain.md (100%) rename vibe/{ => 2026-09}/2026-09-11-3-vfs-foundation.md (100%) rename vibe/{ => 2026-09}/2026-09-12-1-test-namespace-vfs-debt.md (100%) rename vibe/{ => 2026-09}/2026-09-12-2-dependency-rules-vfs-hook.md (100%) rename vibe/{ => 2026-09}/2026-09-12-3-workshop-server-decomposition.md (100%) rename vibe/{ => 2026-09}/2026-09-12-4-workshop-debt-removal.md (100%) rename vibe/{ => 2026-09}/2026-09-12-5-one-door-promptforge-api.md (100%) rename vibe/{ => 2026-09}/2026-09-13-1-capabilities-global-naming.md (100%) rename vibe/{ => 2026-09}/2026-09-14-1-debt-removal-capabilities-follow-ups.md (100%) rename vibe/{ => 2026-09}/2026-09-14-2-provider-model-sheets.md (100%) rename vibe/{ => 2026-09}/2026-09-14-3-fix-previous-sheet-swallow.md (100%) rename vibe/{ => 2026-09}/2026-09-14-4-provider-expansion-local-run.md (100%) rename vibe/{ => 2026-09}/2026-09-14-5-models-aggregation-workflow.md (100%) rename vibe/{ => 2026-09}/2026-09-14-6-model-taxonomy-cloud-ui.md (100%) rename vibe/{ => 2026-09}/2026-09-15-1-cloud-sheet-debt-removal.md (100%) rename vibe/{ => 2026-09}/2026-09-15-2-profiles-gate-local.md (100%) rename vibe/{ => 2026-09}/2026-09-15-3-profiles-debt-removal.md (100%) rename vibe/{ => 2026-09}/2026-09-15-4-flat-sources.md (100%) rename vibe/{ => 2026-09}/2026-09-15-5-workshop-menu-overhaul.md (100%) rename vibe/{ => 2026-09}/2026-09-16-1-turso-workspace-files.md (100%) rename vibe/{ => 2026-09}/2026-09-16-2-workshop-ui-state.md (100%) rename vibe/{ => 2026-09}/2026-09-16-3-workspace-debt-removal.md (100%) rename vibe/{ => 2026-09}/2026-09-17-1-promptforge-family-reorg.md (100%) rename vibe/{ => 2026-09}/2026-09-17-2-gateway-family-reorg.md (100%) rename vibe/{ => 2026-09}/2026-09-17-3-gateway-app-decomp.md (100%) rename vibe/{ => 2026-09}/2026-09-17-4-ci-repair-state-accessors.md (100%) rename vibe/{ => 2026-09}/2026-09-17-5-workshop-family-reorg.md (100%) rename vibe/{ => 2026-09}/2026-09-17-6-minimal-run-window.md (100%) rename vibe/{ => 2026-09}/2026-09-18-1-single-rustls-backend.md (100%) rename vibe/{ => 2026-09}/2026-09-18-2-remove-tool-picker.md (100%) rename vibe/{ => 2026-09}/2026-09-18-3-empty-zone-groups.md (100%) rename vibe/{ => 2026-09}/2026-09-18-4-sans-io-engine-harness.md (100%) rename vibe/{ => 2026-09}/2026-09-19-1-chatbox-extraction.md (100%) diff --git a/crates/gateway/protocol/src/upstream.rs b/crates/gateway/protocol/src/upstream.rs index b9d5e7be..6a375be9 100644 --- a/crates/gateway/protocol/src/upstream.rs +++ b/crates/gateway/protocol/src/upstream.rs @@ -291,7 +291,7 @@ impl OpenAiUpstream { "upstream returned a server error" ); } else { - tracing::debug!( + tracing::info!( status = status.as_u16(), code = %code, r#type = %kind, diff --git a/vibe/2026-09-10-1-unified-prompt-model.md b/vibe/2026-09/2026-09-10-1-unified-prompt-model.md similarity index 100% rename from vibe/2026-09-10-1-unified-prompt-model.md rename to vibe/2026-09/2026-09-10-1-unified-prompt-model.md diff --git a/vibe/2026-09-10-2-debt-fixes.md b/vibe/2026-09/2026-09-10-2-debt-fixes.md similarity index 100% rename from vibe/2026-09-10-2-debt-fixes.md rename to vibe/2026-09/2026-09-10-2-debt-fixes.md diff --git a/vibe/2026-09-11-1-rulebook-debt-tiers.md b/vibe/2026-09/2026-09-11-1-rulebook-debt-tiers.md similarity index 100% rename from vibe/2026-09-11-1-rulebook-debt-tiers.md rename to vibe/2026-09/2026-09-11-1-rulebook-debt-tiers.md diff --git a/vibe/2026-09-11-2-unify-toolchain.md b/vibe/2026-09/2026-09-11-2-unify-toolchain.md similarity index 100% rename from vibe/2026-09-11-2-unify-toolchain.md rename to vibe/2026-09/2026-09-11-2-unify-toolchain.md diff --git a/vibe/2026-09-11-3-vfs-foundation.md b/vibe/2026-09/2026-09-11-3-vfs-foundation.md similarity index 100% rename from vibe/2026-09-11-3-vfs-foundation.md rename to vibe/2026-09/2026-09-11-3-vfs-foundation.md diff --git a/vibe/2026-09-12-1-test-namespace-vfs-debt.md b/vibe/2026-09/2026-09-12-1-test-namespace-vfs-debt.md similarity index 100% rename from vibe/2026-09-12-1-test-namespace-vfs-debt.md rename to vibe/2026-09/2026-09-12-1-test-namespace-vfs-debt.md diff --git a/vibe/2026-09-12-2-dependency-rules-vfs-hook.md b/vibe/2026-09/2026-09-12-2-dependency-rules-vfs-hook.md similarity index 100% rename from vibe/2026-09-12-2-dependency-rules-vfs-hook.md rename to vibe/2026-09/2026-09-12-2-dependency-rules-vfs-hook.md diff --git a/vibe/2026-09-12-3-workshop-server-decomposition.md b/vibe/2026-09/2026-09-12-3-workshop-server-decomposition.md similarity index 100% rename from vibe/2026-09-12-3-workshop-server-decomposition.md rename to vibe/2026-09/2026-09-12-3-workshop-server-decomposition.md diff --git a/vibe/2026-09-12-4-workshop-debt-removal.md b/vibe/2026-09/2026-09-12-4-workshop-debt-removal.md similarity index 100% rename from vibe/2026-09-12-4-workshop-debt-removal.md rename to vibe/2026-09/2026-09-12-4-workshop-debt-removal.md diff --git a/vibe/2026-09-12-5-one-door-promptforge-api.md b/vibe/2026-09/2026-09-12-5-one-door-promptforge-api.md similarity index 100% rename from vibe/2026-09-12-5-one-door-promptforge-api.md rename to vibe/2026-09/2026-09-12-5-one-door-promptforge-api.md diff --git a/vibe/2026-09-13-1-capabilities-global-naming.md b/vibe/2026-09/2026-09-13-1-capabilities-global-naming.md similarity index 100% rename from vibe/2026-09-13-1-capabilities-global-naming.md rename to vibe/2026-09/2026-09-13-1-capabilities-global-naming.md diff --git a/vibe/2026-09-14-1-debt-removal-capabilities-follow-ups.md b/vibe/2026-09/2026-09-14-1-debt-removal-capabilities-follow-ups.md similarity index 100% rename from vibe/2026-09-14-1-debt-removal-capabilities-follow-ups.md rename to vibe/2026-09/2026-09-14-1-debt-removal-capabilities-follow-ups.md diff --git a/vibe/2026-09-14-2-provider-model-sheets.md b/vibe/2026-09/2026-09-14-2-provider-model-sheets.md similarity index 100% rename from vibe/2026-09-14-2-provider-model-sheets.md rename to vibe/2026-09/2026-09-14-2-provider-model-sheets.md diff --git a/vibe/2026-09-14-3-fix-previous-sheet-swallow.md b/vibe/2026-09/2026-09-14-3-fix-previous-sheet-swallow.md similarity index 100% rename from vibe/2026-09-14-3-fix-previous-sheet-swallow.md rename to vibe/2026-09/2026-09-14-3-fix-previous-sheet-swallow.md diff --git a/vibe/2026-09-14-4-provider-expansion-local-run.md b/vibe/2026-09/2026-09-14-4-provider-expansion-local-run.md similarity index 100% rename from vibe/2026-09-14-4-provider-expansion-local-run.md rename to vibe/2026-09/2026-09-14-4-provider-expansion-local-run.md diff --git a/vibe/2026-09-14-5-models-aggregation-workflow.md b/vibe/2026-09/2026-09-14-5-models-aggregation-workflow.md similarity index 100% rename from vibe/2026-09-14-5-models-aggregation-workflow.md rename to vibe/2026-09/2026-09-14-5-models-aggregation-workflow.md diff --git a/vibe/2026-09-14-6-model-taxonomy-cloud-ui.md b/vibe/2026-09/2026-09-14-6-model-taxonomy-cloud-ui.md similarity index 100% rename from vibe/2026-09-14-6-model-taxonomy-cloud-ui.md rename to vibe/2026-09/2026-09-14-6-model-taxonomy-cloud-ui.md diff --git a/vibe/2026-09-15-1-cloud-sheet-debt-removal.md b/vibe/2026-09/2026-09-15-1-cloud-sheet-debt-removal.md similarity index 100% rename from vibe/2026-09-15-1-cloud-sheet-debt-removal.md rename to vibe/2026-09/2026-09-15-1-cloud-sheet-debt-removal.md diff --git a/vibe/2026-09-15-2-profiles-gate-local.md b/vibe/2026-09/2026-09-15-2-profiles-gate-local.md similarity index 100% rename from vibe/2026-09-15-2-profiles-gate-local.md rename to vibe/2026-09/2026-09-15-2-profiles-gate-local.md diff --git a/vibe/2026-09-15-3-profiles-debt-removal.md b/vibe/2026-09/2026-09-15-3-profiles-debt-removal.md similarity index 100% rename from vibe/2026-09-15-3-profiles-debt-removal.md rename to vibe/2026-09/2026-09-15-3-profiles-debt-removal.md diff --git a/vibe/2026-09-15-4-flat-sources.md b/vibe/2026-09/2026-09-15-4-flat-sources.md similarity index 100% rename from vibe/2026-09-15-4-flat-sources.md rename to vibe/2026-09/2026-09-15-4-flat-sources.md diff --git a/vibe/2026-09-15-5-workshop-menu-overhaul.md b/vibe/2026-09/2026-09-15-5-workshop-menu-overhaul.md similarity index 100% rename from vibe/2026-09-15-5-workshop-menu-overhaul.md rename to vibe/2026-09/2026-09-15-5-workshop-menu-overhaul.md diff --git a/vibe/2026-09-16-1-turso-workspace-files.md b/vibe/2026-09/2026-09-16-1-turso-workspace-files.md similarity index 100% rename from vibe/2026-09-16-1-turso-workspace-files.md rename to vibe/2026-09/2026-09-16-1-turso-workspace-files.md diff --git a/vibe/2026-09-16-2-workshop-ui-state.md b/vibe/2026-09/2026-09-16-2-workshop-ui-state.md similarity index 100% rename from vibe/2026-09-16-2-workshop-ui-state.md rename to vibe/2026-09/2026-09-16-2-workshop-ui-state.md diff --git a/vibe/2026-09-16-3-workspace-debt-removal.md b/vibe/2026-09/2026-09-16-3-workspace-debt-removal.md similarity index 100% rename from vibe/2026-09-16-3-workspace-debt-removal.md rename to vibe/2026-09/2026-09-16-3-workspace-debt-removal.md diff --git a/vibe/2026-09-17-1-promptforge-family-reorg.md b/vibe/2026-09/2026-09-17-1-promptforge-family-reorg.md similarity index 100% rename from vibe/2026-09-17-1-promptforge-family-reorg.md rename to vibe/2026-09/2026-09-17-1-promptforge-family-reorg.md diff --git a/vibe/2026-09-17-2-gateway-family-reorg.md b/vibe/2026-09/2026-09-17-2-gateway-family-reorg.md similarity index 100% rename from vibe/2026-09-17-2-gateway-family-reorg.md rename to vibe/2026-09/2026-09-17-2-gateway-family-reorg.md diff --git a/vibe/2026-09-17-3-gateway-app-decomp.md b/vibe/2026-09/2026-09-17-3-gateway-app-decomp.md similarity index 100% rename from vibe/2026-09-17-3-gateway-app-decomp.md rename to vibe/2026-09/2026-09-17-3-gateway-app-decomp.md diff --git a/vibe/2026-09-17-4-ci-repair-state-accessors.md b/vibe/2026-09/2026-09-17-4-ci-repair-state-accessors.md similarity index 100% rename from vibe/2026-09-17-4-ci-repair-state-accessors.md rename to vibe/2026-09/2026-09-17-4-ci-repair-state-accessors.md diff --git a/vibe/2026-09-17-5-workshop-family-reorg.md b/vibe/2026-09/2026-09-17-5-workshop-family-reorg.md similarity index 100% rename from vibe/2026-09-17-5-workshop-family-reorg.md rename to vibe/2026-09/2026-09-17-5-workshop-family-reorg.md diff --git a/vibe/2026-09-17-6-minimal-run-window.md b/vibe/2026-09/2026-09-17-6-minimal-run-window.md similarity index 100% rename from vibe/2026-09-17-6-minimal-run-window.md rename to vibe/2026-09/2026-09-17-6-minimal-run-window.md diff --git a/vibe/2026-09-18-1-single-rustls-backend.md b/vibe/2026-09/2026-09-18-1-single-rustls-backend.md similarity index 100% rename from vibe/2026-09-18-1-single-rustls-backend.md rename to vibe/2026-09/2026-09-18-1-single-rustls-backend.md diff --git a/vibe/2026-09-18-2-remove-tool-picker.md b/vibe/2026-09/2026-09-18-2-remove-tool-picker.md similarity index 100% rename from vibe/2026-09-18-2-remove-tool-picker.md rename to vibe/2026-09/2026-09-18-2-remove-tool-picker.md diff --git a/vibe/2026-09-18-3-empty-zone-groups.md b/vibe/2026-09/2026-09-18-3-empty-zone-groups.md similarity index 100% rename from vibe/2026-09-18-3-empty-zone-groups.md rename to vibe/2026-09/2026-09-18-3-empty-zone-groups.md diff --git a/vibe/2026-09-18-4-sans-io-engine-harness.md b/vibe/2026-09/2026-09-18-4-sans-io-engine-harness.md similarity index 100% rename from vibe/2026-09-18-4-sans-io-engine-harness.md rename to vibe/2026-09/2026-09-18-4-sans-io-engine-harness.md diff --git a/vibe/2026-09-19-1-chatbox-extraction.md b/vibe/2026-09/2026-09-19-1-chatbox-extraction.md similarity index 100% rename from vibe/2026-09-19-1-chatbox-extraction.md rename to vibe/2026-09/2026-09-19-1-chatbox-extraction.md From 34191d5d1a5e81b06395c77dade5ac7e01e1438c Mon Sep 17 00:00:00 2001 From: Vinnie Falco Date: Tue, 22 Sep 2026 14:51:55 -0700 Subject: [PATCH 07/19] =?UTF-8?q?=EF=BB=BFEnable=20exact=20JSON=20float=20?= =?UTF-8?q?parsing=20workspace-wide?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Enabling exact float parsing on the workspace JSON dependency makes a payload read back as the identical value its stored text represents, so replay can compare re-executed results against stored records without a silent loss of precision. The generated workspace feature set is regenerated to carry the same setting, and a build-only dependency that the regenerated graph no longer requires is dropped. - `Cargo.toml` enables exact parsing once on the workspace JSON dependency rather than at each call site, so every consumer inherits the guarantee. - `crates/workspace-hack/Cargo.toml` is regenerated to carry the same feature, and the regenerated set no longer includes `time-macros`. Plan: vibe/2026-09-22-3-replay-json-fidelity.md --- Cargo.lock | 1 - Cargo.toml | 4 +- crates/workspace-hack/Cargo.toml | 5 +- vibe/2026-09-22-3-replay-json-fidelity.md | 242 ++++++++++++++++++++++ vibe/ACTIVE | 1 + 5 files changed, 248 insertions(+), 5 deletions(-) create mode 100644 vibe/2026-09-22-3-replay-json-fidelity.md create mode 100644 vibe/ACTIVE diff --git a/Cargo.lock b/Cargo.lock index 6f6ae8ea..a69df432 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -8740,7 +8740,6 @@ dependencies = [ "tar", "tauri-utils", "time", - "time-macros", "tinystr", "tokio", "tokio-util", diff --git a/Cargo.toml b/Cargo.toml index f2b18f9a..8a27de51 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -75,7 +75,9 @@ workspace-hack = { path = "crates/workspace-hack", version = "0.1" } pulldown-cmark = "0.12" serde = { version = "1", features = ["derive"] } serde_yaml_ng = "0.10" -serde_json = "1" +# `float_roundtrip` makes `from_str` reproduce the exact `f64` that +# `to_string` printed, so a payload the run log stores reads back bit-for-bit. +serde_json = { version = "1", features = ["float_roundtrip"] } # 0.13 rather than 0.12 so the tree links exactly one rustls crypto backend: # aws-lc-rs, the one Tauri already selects. 0.13 has no `ring` # option, so a bump cannot re-enable it; CI checks that `ring` stays out of diff --git a/crates/workspace-hack/Cargo.toml b/crates/workspace-hack/Cargo.toml index 7d0f8026..7e5af2a6 100644 --- a/crates/workspace-hack/Cargo.toml +++ b/crates/workspace-hack/Cargo.toml @@ -53,7 +53,7 @@ schemars = { version = "0.8", features = ["preserve_order", "url", "uuid1"] } semver = { version = "1", features = ["serde"] } serde = { version = "1", features = ["alloc", "derive", "rc"] } serde_core = { version = "1", features = ["alloc", "rc"] } -serde_json = { version = "1", features = ["alloc", "raw_value", "unbounded_depth"] } +serde_json = { version = "1", features = ["alloc", "float_roundtrip", "raw_value", "unbounded_depth"] } simd-adler32 = { version = "0.3" } smallvec = { version = "1", default-features = false, features = ["const_new", "union"] } stable_deref_trait = { version = "1" } @@ -121,7 +121,7 @@ schemars = { version = "0.8", features = ["preserve_order", "url", "uuid1"] } semver = { version = "1", features = ["serde"] } serde = { version = "1", features = ["alloc", "derive", "rc"] } serde_core = { version = "1", features = ["alloc", "rc"] } -serde_json = { version = "1", features = ["alloc", "raw_value", "unbounded_depth"] } +serde_json = { version = "1", features = ["alloc", "float_roundtrip", "raw_value", "unbounded_depth"] } simd-adler32 = { version = "0.3" } smallvec = { version = "1", default-features = false, features = ["const_new", "union"] } stable_deref_trait = { version = "1" } @@ -132,7 +132,6 @@ syn-f595c2ba2a3f28df = { package = "syn", version = "2", features = ["extra-trai sync_wrapper = { version = "1", default-features = false, features = ["futures"] } tauri-utils = { version = "2", default-features = false, features = ["build-2", "compression", "resources"] } time = { version = "0.3", features = ["macros", "serde-human-readable"] } -time-macros = { version = "0.2", default-features = false, features = ["formatting", "parsing", "serde"] } tinystr = { version = "0.8", default-features = false, features = ["alloc", "zerovec"] } tokio = { version = "1", features = ["fs", "io-util", "macros", "net", "rt-multi-thread", "signal", "test-util"] } tracing = { version = "0.1", features = ["log"] } diff --git a/vibe/2026-09-22-3-replay-json-fidelity.md b/vibe/2026-09-22-3-replay-json-fidelity.md new file mode 100644 index 00000000..b5f19011 --- /dev/null +++ b/vibe/2026-09-22-3-replay-json-fidelity.md @@ -0,0 +1,242 @@ +--- +name: Replay-grade JSON fidelity +overview: Make every payload the run log stores round-trip bit-for-bit (exact float parsing, short timings, canonical key order, pinned by tests), make Lua `pairs`/`next` iterate in a deterministic sorted order, and write the rule down in AGENTS.md and the log crate's invariants. +todos: + - id: float-roundtrip + content: Enable serde_json float_roundtrip on the workspace dependency; regenerate workspace-hack with cargo hakari generate; hakari verify passes + status: pending + - id: round-timings + content: Round ClientTiming fields to microseconds in model-client read.rs (duration_ms + mean_itl_ms) with a bit-exact round-trip test + status: pending + - id: log-fidelity-test + content: "Add harness-log tests/it/fidelity.rs covering awkward floats, key order, nested Value, real AssistantReply metrics; add the round-trip bullet to harness-log ## Invariants" + status: pending + - id: sort-key-extract + content: Extract SortKey and sort_key helper from collection.rs into a shared pub(crate) module; collection_members reuses it + status: pending + - id: sorted-pairs + content: Add install_deterministic_iteration replacing pairs/next with sorted iteration (honor __pairs, skip cleared keys); call after harden in SectionVm::new; tests + status: pending + - id: pairs-audit + content: Audit Rust-side table.pairs() walks that produce ordered Vecs; sort or record each site + status: pending + - id: agents-rule + content: Add the one-line round-trip rule to AGENTS.md Engineering; add the sorted pairs sentence to the user guide + status: pending + - id: verify + content: fmt, clippy, hakari verify, build-xtask, docs, full nextest + doctests, 20x loop of the end_to_end test, mdbook build + status: pending +isProject: false +--- + +# Replay-grade JSON fidelity and deterministic Lua iteration + + + +## Product Requirements + +A run's stored payloads must read back as the identical JSON value so that replay can compare re-executed results against them, and Lua table iteration must visit keys in the same order in every process. Today float text is not always parsed exactly, three timing fields carry nanosecond noise that need not survive, and Lua `pairs`/`next` walk a table in an unspecified hash order. This plan makes the log round-trip bit-for-bit, rounds those timings at their source, replaces `pairs`/`next` with sorted iteration, and records the rules. + +- Problem and users: the run log writes a record's payload as JSON text and parses it back for replay, whose vocabulary lives at `crates/promptforge-api-types/src/replay.rs`. `serde_json`'s default parser is not exact, so `3.9078000000000004` reads back as `3.9078`, and Lua `pairs`/`next` iterate a table in an unspecified hash order, so two processes can visit the same table differently. The users are the replay path and any host that compares stored payloads. +- Goals: exact float parsing across the workspace; timing values rounded to microseconds at their source so their text form is short and parses exactly; a log fidelity test pinning `Value` identity and canonical text; sorted, deterministic `pairs`/`next`; and the round-trip rule and the iteration order written into the repository's author-facing guidance and the Lua language guide. +- Non-goals: no fork of `mlua`; no use of a configurable Lua hash seed; no replay implementation; no change to the Lua standard-library surface other than iteration order. +- Success criteria: `from_str(to_string(value))` equals the original `Value` for awkward floats, nested objects built in unsorted insertion order, and a real `AssistantReply` carrying `CallMetrics`; `pairs`/`next` yield the identical order in two fresh VMs for the same table; `cargo hakari verify` passes. +- Constraints: `serde_json` feature unification is owned by `crates/workspace-hack`; `Value::Object` is a `BTreeMap` because no crate enables `preserve_order`, so key order is already canonical and must stay so; non-finite floats cannot be stored faithfully, so producers reject them at the source. +- Open questions: None + +## Functional Specification + +The run log round-trips a payload textually, and a Lua author iterates a table deterministically. The log writes each record's payload with `serde_json::to_string` (`crates/harness/log/src/append.rs:96`) and reads it with `serde_json::from_str` (`crates/harness/log/src/read.rs:97`); the payload is already a `serde_json::Value` before it reaches the writer. Lua iteration becomes a pure function of the table's contents rather than of its internal hash state. + +- Actors and workflows: the log writer serializes a `Value` payload to TEXT and the reader parses it back; a Lua author walks a table with `pairs` or `next` inside a section VM (`crates/promptforge/lua/src/vm.rs`). +- Inputs and outputs: input is an event payload (a `serde_json::Value`) or a Lua table; output is stored text plus a parsed `Value` equal to the input, and a key sequence that depends only on the table's contents. +- States and validation: a stored payload holds only finite numbers and canonical (sorted) object keys; `pairs`/`next` honor a `__pairs` metamethod, visit the array part first in index order, then hash keys by sort order, and skip any key whose value has become `nil` during traversal. +- Errors and recovery: a non-finite number is not storable as itself, so producers reject it before the log sees it; a value the parser cannot round-trip is a defect the fidelity test catches rather than a silent loss. +- Security and privacy behavior: no new data is stored and no new surface is exposed; the change removes a silent-loss path (a float whose text cannot round-trip) rather than adding one. +- Acceptance criteria: the fidelity test passes for the listed payloads; the iteration tests pass; the end-to-end agent run passes repeatedly. + + + + +## Technical Design + +Three cross-module changes make replay comparison sound. The workspace enables `serde_json`'s `float_roundtrip` feature so every `from_str` is exact. The model client rounds its derived timing fields to microseconds so the values that reach the log have short, exactly-parsing text. The Lua VM replaces `pairs`/`next` with sorted iteration so table traversal is deterministic without touching `mlua`. + +```mermaid +flowchart LR + Engine[engine event] -->|to_string| Text[log TEXT] + Text -->|from_str| Back[Value] + Back -.->|must equal| Val[original Value] +``` + +- Architecture: exactness is fixed once at the workspace dependency level rather than per call site; determinism is fixed at the Lua runtime boundary rather than inside `mlua`; timing rounding happens where the derived value is computed, so the logged text is short for every consumer. +- Modules and interfaces: `crates/promptforge/model-client/src/client/read.rs` computes `duration_ms` as `duration.as_secs_f64() * 1000.0` (lines 166-169) and `mean_itl_ms` from it (lines 152-157); both become whole-microsecond values. `crates/promptforge/lua/src/collection.rs` already defines a private `SortKey` (lines 24-55) used by the enumeration path at lines 124-125 and sorted at line 161; that ordering is extracted to a shared crate-internal helper and a new iteration installer reuses it. +- File and public API changes: the workspace root `Cargo.toml` (line 78) gains the `float_roundtrip` feature; `crates/workspace-hack/Cargo.toml` is regenerated. The Lua change adds crate-internal modules only (`collection-order`/`order` and `iteration`) and installs them in `SectionVm::new` after `harden` (`crates/promptforge/lua/src/vm.rs:275`). The compile-only VM (`crates/promptforge/lua/src/program.rs:13`) is untouched. No public API changes. +- Data, persistence, failure, security, and privacy constraints: the stored payload format does not change shape; exact parsing makes existing text read back exactly; object keys stay sorted because `Value::Object` is a `BTreeMap` (no crate enables `preserve_order`); a payload containing a non-finite number is impossible to store as itself, so the producers that can emit one already reject it at the source - temperature through `crates/promptforge/model-client/src/model/options.rs:23-29` and timer seconds through `crates/promptforge/lua/src/protocol/parse-tasks.rs:56-60` with a defensive repeat in `crates/promptforge-api-runtime/src/execute/scheduler/timer.rs:53-58`. + + + + +## Testing Plan + +Tests pin the log round-trip and the iteration order, and the change is gated by the workspace checks and a flake loop. The fidelity test covers the value shapes that fail today; the iteration tests cover the ordering rules; the exit criteria run the full suite and the guide build. + +- Unit: in `crates/promptforge/model-client/src/client/read-tests.rs`, a `ClientTiming` built from a `Duration` with nanosecond noise serializes to a short decimal and `from_str(to_string(v))` equals `v` for all three fields, adjusting the existing `EPSILON` assertions (lines 129-131). In the Lua crate, `iteration-tests.rs` asserts string keys iterate in byte order regardless of insertion order, the array part precedes the hash part, mixed key types order bool < number < string, `__pairs` is honored, `next(t) == nil` on an empty table, a cleared field is skipped mid-traversal, and two fresh VMs produce the identical order for the same table. +- Integration and end-to-end: a new `crates/harness/log/tests/it/fidelity.rs` appends records whose payloads hold awkward floats (`0.1 + 0.2`, `3.9078000000000004`, `1e-7`, `1e21`, `f64::MAX`, negative zero), nested objects built with keys in non-sorted insertion order, arrays of mixed numbers, and a real `Event::AssistantReply` with `CallMetrics`; reading back asserts `stored.payload == original` and `to_string(&stored.payload) == to_string(&original)`. Run `cargo nextest run -p harness-models a_prepared_run_drives_end_to_end` twenty times in a loop; all must pass. +- Regression, security, and performance: confirm no crate enables `preserve_order` (so key order stays canonical) and none enables `float_roundtrip` today; add a unit test in `crates/promptforge-api-types/src/metrics.rs` proving that serializing a non-finite `f64` yields `null`, documenting why producers reject non-finite values at the source; audit every Rust-side `table.pairs()` walk that produces an ordered `Vec` rather than a `Value` (known sites: `crates/promptforge/lua/src/collection.rs`, already sorted, and `crates/promptforge/lua/src/tools/decode.rs:115`, which sinks into an object and is canonical) and sort or record each. +- Exit criteria: `cargo fmt --all --check`; workspace clippy with `-D warnings`; `cargo hakari verify`; `cargo test -p build-xtask` (new files stay within the repository's file-size convention); rustdoc with `RUSTDOCFLAGS="-D warnings"`; the full nextest suite plus doctests; the twenty-run flake loop; `mdbook build guide`. + + + + +## Decision Record + +The design settles four calls and rejects two alternatives. Determinism is achieved by replacing Lua's `pairs`/`next` with sorted iteration rather than by controlling the runtime's hash seed, because `mlua` seeds the state itself. Exactness is achieved by enabling exact parsing workspace-wide and by rounding derived timings at their source. + +- Decisions: + - Replace `pairs`/`next` with sorted iteration: `mlua` calls `luaL_makeseed` itself and exposes no hash-seed override, so determinism cannot be configured; sorting removes the dependency on internal hash state. User's words: selected replacing `pairs`/`next` with sorted iteration rather than forking `mlua`. + - Enable `serde_json`'s `float_roundtrip` for the workspace and regenerate `crates/workspace-hack`: this fixes every `from_str` at once with no code change. User's plan decision. + - Round `duration_ms` and `mean_itl_ms` to whole microseconds at their source: the logged text is then short and parses exactly even independent of the feature, and no downstream consumer sees noise. User's plan decision. + - Keep canonical `BTreeMap` key order and pin it by test instead of switching to insertion order. User's plan decision. +- Rejected alternatives: + - Fork `mlua` to control the Lua hash seed: sorted iteration makes it unnecessary for determinism; revisit only as an upstream `hash_seed` contribution. + - Enable `preserve_order` to own key order: `Value::Object` is already canonical as a `BTreeMap`, and `preserve_order` adds a dependency and insertion-order semantics; revisit only if ordered keys are ever required. + - Post-process logged text to repair floats: rejected in favor of exact parsing plus source rounding. +- Assumptions, risks, and notes: + - Note: `.config/hakari.toml` states that CI runs `cargo hakari verify`, but no workflow under `.github/workflows` currently invokes it, so the check is run explicitly by this plan's exit criteria. + - Note: `crates/harness/log/src/append.rs:96` serializes an already-built `Value` with `to_string`; the `to_value` conversion happens where the event is converted, not in the writer. + - Risk: enabling `float_roundtrip` changes parsing for every crate's JSON; the fidelity test and the full suite bound the risk. + - Assumption: no `HashMap`/`HashSet` appears in any logged type, and tool advertisement order is registration order, so key order is deterministic already. + - Note: the Lua crate carries no file-size marker, but new files stay small per the repository convention. + +### Deferred and Out of Scope + +- Deferred: an `mlua` upstream `hash_seed` option (revisit as a separate upstream contribution); reconciling `vibe/scratch/vibe-ledger.md` to the superseded-design history (separate housekeeping). +- Out of scope: building replay itself; changing the Lua standard-library surface beyond iteration order. + + + + +## Project Survey + +- Status: complete +- Build command: `cargo build --locked -p gateway` (the workspace default member; a bare `cargo build` builds only the gateway). Install the web UI dependencies first with `npm ci --prefix crates/workshop/ui` and `npm ci --prefix crates/gateway/config-ui/ui`, since both are bundled by esbuild during the Cargo build. The desktop app uses `cargo workshop`. +- Focused test command pattern: `cargo nextest run --locked -p ` (add `--all-features` for workspace crates; the workshop crates drop it). One integration case: `cargo test --locked -p --test it `. Doctests are not run by nextest, so they are separate: `cargo test -p --doc`. +- Component test command pattern: `cargo nextest run --locked -p --all-features`; the workshop partition `cargo nextest run --locked -p workshop -p workshop-server -p workshop-server-api`, plus `cargo nextest run --locked -p workshop-server --features headless`; the structural and boundary harness `cargo test -p build-xtask`; and the TypeScript package `npm test` (`node --test`) inside `crates/workshop/ui` and `crates/gateway/config-ui/ui`. +- Full-suite test command: `cargo nextest run --locked --workspace --exclude workshop --exclude workshop-server --exclude workshop-server-api --all-features`, then `cargo test --workspace --exclude workshop --exclude workshop-server --exclude workshop-server-api --all-features --doc`; the workshop crates separately via `cargo nextest run --locked -p workshop -p workshop-server -p workshop-server-api` and `cargo test --doc -p workshop -p workshop-server -p workshop-server-api`; plus `cargo test -p build-xtask`. +- Linter command: `cargo clippy --workspace --exclude workshop --exclude workshop-server --exclude workshop-server-api --all-targets --all-features -- -D warnings`; workshop: `cargo clippy -p workshop -p workshop-server -p workshop-server-api --all-targets -- -D warnings`. Supply-chain checks are `cargo deny check` and `cargo audit`. Clippy is a superset of `cargo check`, so never run a standalone `cargo check --workspace`; the one exception is the headless build-shape gate `cargo check -p gateway --no-default-features`. +- Formatter check command: `cargo fmt --all --check`. +- Docs command: `RUSTDOCFLAGS="-D warnings"` with `cargo doc --workspace --no-deps --all-features --exclude workshop --exclude workshop-server --exclude workshop-server-api`; the user guide builds with `mdbook build guide`, and its generated SUMMARY/index files regenerate with `cargo run -p build-user-guide`. +- Test placement and naming conventions: Rust unit tests are kebab sibling files beside the module they test, wired by an explicit path attribute (`src/auth.rs` with `#[path = "auth-tests.rs"] mod tests;`); a source group of three or more files becomes a `foo/` subdirectory and flattens back below three. Rust integration tests live under `tests/it/` with `main.rs` plus feature modules (`boot.rs`, `chat.rs`, `support.rs`) and run as the `it` target. Test function names are full snake_case sentences describing the behavior. TypeScript tests live in `test/**/*.mjs` under `crates/workshop/ui` or beside sources as `src/**/*.test.mjs` in `crates/gateway/config-ui/ui`, both run under `node --test`. `.config/nextest.toml` sets a 60s slow-timeout that terminates after 3 periods, a 250ms leak-timeout, and a `heavy` group (max 8 threads, 4 required per test) for the whisper STT crates. +- Directory map: the root holds `Cargo.toml`/`Cargo.lock` (workspace, resolver 3, default member `crates/gateway/app`), `rust-toolchain.toml` (stable), `rustfmt.toml` (edition 2024 style), `clippy.toml`, `deny.toml`, `dist-workspace.toml`, `AGENTS.md`, `README.md`, plus `.config/` (hakari and nextest), `.cargo/`, `.cursor/`, `.githooks/`, `.github/` (workflows and fixtures), `crates/` (all Rust crates and the `shared-ui` TypeScript+CSS package), `guide/` (mdBook sources), `images/`, `local/`, `prompts/`, `tools/` (Node scripts and a harness tool markdown), `vibe/` (architecture notes and dated plan logs), and `target/`/`target-msrv/` (build output). `crates/` splits into public root crates (`promptforge-api-runtime`, `promptforge-api-types`, `gateway-api-types`, `gateway-api-discovery`, `harness-api`, `shared-vfs`, `shared-loopback`, `shared-error-source`, `workspace-hack`), four manifestless family containers (`crates/promptforge/`: lua, parser, store, vfs, model-client; `crates/gateway/`: app, config, routing, local, web-search, logging, progress, protocol, cloud-providers, config-ui, and a nested `stt/`; `crates/workshop/`: shell (package `workshop`), server, server-api, gateway, menu, protocol, registry, status, support, user-state, workspace, ui; `crates/harness/`: runner, models, capabilities, log, sessions, web, webfetch, web-search), and `build-*` meta tooling (`build-xtask`, `build-ui`, `build-user-guide`, `build-workshop`, `build-llama-cuda`). +- Component boundaries: the executor (`promptforge-api-runtime`) is a deterministic sans-I/O state machine driven by `Run::new`, `step`, `resume`, and `cancel` that depends on the store, the Lua VM boundary, and shared substrate; the harness is its only production host, owning the tokio runtime, effect performers, the model HTTP client, the capability registry, sessions, and the Turso run log, with `harness-api` as its single public crate; the gateway is an independent server process owning model routing and provider access behind the two public crates `gateway-api-types` and `gateway-api-discovery`; the CLI and Workshop UI are thin adapters that drive runs through those public surfaces. Dependencies point one way, shell to features to services to vocabulary; the family containers are private (no outside crate may depend into them, only their root public crate is importable), promptforge-* must not depend on gateway/workshop/harness crates, gateway crates must not depend on promptforge/workshop crates, and workshop crates reach the harness only through `harness-api`. `cargo test -p build-xtask` enforces the tier graph, container privacy, the mandatory `## Invariants` marker, lint inheritance, and the 500-line ceiling. +- Conventions summary: Rust edition 2024 on the stable toolchain with workspace-inherited lints (clippy `all` and `pedantic` denied, `unwrap_used`/`expect_used` denied, `unsafe_code` forbidden, `missing_docs` warned); behavior changes ship with their tests in the same change and product tests are preserved during refactors. Every `workshop-*` and `harness-*` crate's lib.rs opens with a `//!` doc containing a `## Invariants` marker, and no file in a marked crate exceeds 500 lines. Source directories are flat by default. Comments explain a non-obvious constraint, and every external workaround cites its upstream issue URL. Errors and status messages are concise and self-contained for model consumption. A Cargo feature gates a real constraint rather than product shape. The TypeScript UI keeps CSS beside its TypeScript and uses `--ws-*` tokens instead of raw values, never uses `localStorage` (persistence goes through the `ui-storage` adapter to server-side state files), and keeps each feature directory self-contained. The guide follows house rules: no em-dash or double-dash, four-backtick code fences, and one-line paragraphs. + + + + +## Execution Instructions + +The work splits into six components in dependency order: workspace JSON exactness, replay-grade log round-trip, deterministic Lua iteration, a hardening audit, author-facing rules, and the verification gate. The workspace feature gates the round-trip result, the shared ordering helper gates the iteration installer, and the audits are independent of both. + + + +### Step 1: Enable exact JSON parsing workspace-wide [completed] + +- Component: workspace JSON exactness + +Enable `serde_json`'s `float_roundtrip` feature on the workspace dependency in `Cargo.toml`, regenerate `crates/workspace-hack/Cargo.toml` with `cargo hakari generate`, and confirm `cargo hakari verify` passes. + + + + + +### Step 2: Round ClientTiming fields to microseconds + +- Component: replay-grade log round-trip + +In `crates/promptforge/model-client/src/client/read.rs`, round `duration_ms` (from `duration.as_secs_f64() * 1000.0`) and `mean_itl_ms` to whole microseconds, with a doc comment naming the round-trip reason. Add a bit-exact round-trip unit test in `crates/promptforge/model-client/src/client/read-tests.rs` and adjust the existing `EPSILON` assertions. + + + + + +### Step 3: Pin log payload fidelity + +- Component: replay-grade log round-trip + +Add `crates/harness/log/tests/it/fidelity.rs` covering awkward floats, nested objects built in non-sorted insertion order, mixed-number arrays, and a real `Event::AssistantReply` carrying `CallMetrics`; assert `stored.payload == original` and matching `to_string` text. Add the round-trip bullet to the `## Invariants` section of `crates/harness/log/src/lib.rs`. + + + + + +### Step 4: Extract the shared ordering helper + +- Component: deterministic Lua iteration + +Extract `SortKey` and a `sort_key` helper from `crates/promptforge/lua/src/collection.rs` into a shared `pub(crate)` module and make `collection_members` reuse it. + + + + + +### Step 5: Install sorted pairs and next + +- Component: deterministic Lua iteration + +Add `install_deterministic_iteration` replacing `pairs`/`next` with sorted iteration that honors `__pairs`, visits the array part first in index order, orders mixed keys bool < number < string, and skips keys cleared to `nil`. Add unit tests in `crates/promptforge/lua/src/iteration-tests.rs`. + + + + + +### Step 6: Activate deterministic iteration in the section VM + +- Component: deterministic Lua iteration + +Call `install_deterministic_iteration` in `SectionVm::new` after `harden` in `crates/promptforge/lua/src/vm.rs` and add the test asserting two fresh VMs yield the identical key order for the same table. + + + + + +### Step 7: Record the non-finite and canonical-order regressions + +- Component: hardening audit + +Add a unit test in `crates/promptforge-api-types/src/metrics.rs` proving a non-finite `f64` serializes to `null`, documenting why producers reject non-finite values at the source, and confirm no crate enables `preserve_order` so `Value::Object` key order stays canonical. + + + + + +### Step 8: Audit ordered pair walks + +- Component: hardening audit + +Audit every Rust-side `table.pairs()` walk that produces an ordered `Vec` rather than a `Value` (known sites: `crates/promptforge/lua/src/collection.rs`, already sorted, and `crates/promptforge/lua/src/tools/decode.rs`), sorting or recording each site. + + + + + +### Step 9: Write the author-facing rules + +- Component: author-facing rules + +Add the round-trip rule to the Engineering section of `AGENTS.md` and the sorted-`pairs` sentence to the Lua language guide under `guide/src`. + + + + + +### Step 10: Run the exit checks + +- Component: verification gate + +Run `cargo fmt --all --check`, workspace clippy with `-D warnings`, `cargo hakari verify`, `cargo test -p build-xtask`, rustdoc with `RUSTDOCFLAGS="-D warnings"`, the full nextest suite plus doctests, the twenty-run loop of `a_prepared_run_drives_end_to_end`, and `mdbook build guide`. + + + + diff --git a/vibe/ACTIVE b/vibe/ACTIVE new file mode 100644 index 00000000..06ec519f --- /dev/null +++ b/vibe/ACTIVE @@ -0,0 +1 @@ +vibe/2026-09-22-3-replay-json-fidelity.md From 23d09a3f902a160b16e4cfe56ba06b9a9ac2e1bd Mon Sep 17 00:00:00 2001 From: Vinnie Falco Date: Tue, 22 Sep 2026 15:00:50 -0700 Subject: [PATCH 08/19] =?UTF-8?q?=EF=BB=BFRound=20client=20timings=20to=20?= =?UTF-8?q?whole=20microseconds?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The model client now rounds each measured timing to a whole microsecond where it is computed, so the text a run log stores is short and parses back exactly on replay. A timing that carried nanosecond noise used to be written as a long decimal expansion that replay did not need. - `round_to_microsecond` is a new pure helper that scales a fractional-millisecond value by a thousand, rounds it, and scales back; `duration_ms` now routes every duration through it. - `mean_itl_ms` is rounded after the division by the interval count, so a repeating quotient is trimmed before it reaches the log. - `ClientTiming` keeps its fields and public shape; only the values it carries are rounded. Design: new pure-function @ crates/promptforge/model-client/src/client/read.rs::round_to_microsecond deps: f64 Design: extends pure-function @ crates/promptforge/model-client/src/client/read.rs::duration_ms deps: Duration Repairs: logged timing text is short and round-trips exactly @ crates/promptforge/model-client/src/client/read.rs::duration_ms - a timing carrying nanosecond noise serialized as a long decimal expansion Plan: vibe/2026-09-22-3-replay-json-fidelity.md --- .../model-client/src/client/read-tests.rs | 59 ++++++++++++++++++- .../model-client/src/client/read.rs | 20 +++++-- vibe/2026-09-22-3-replay-json-fidelity.md | 2 +- 3 files changed, 72 insertions(+), 9 deletions(-) diff --git a/crates/promptforge/model-client/src/client/read-tests.rs b/crates/promptforge/model-client/src/client/read-tests.rs index 1be33926..3ed1c42a 100644 --- a/crates/promptforge/model-client/src/client/read-tests.rs +++ b/crates/promptforge/model-client/src/client/read-tests.rs @@ -12,6 +12,7 @@ use serde_json::json; use super::*; use crate::client::CompletionResult; use crate::model::CompletionErrorKind; +use promptforge_api_types::metrics::ClientTiming; /// A chunk source over canned chunks; it never pends, so the tests need /// no executor. @@ -126,9 +127,61 @@ fn read_completion_stream_reassembles_the_turn_and_times_it_on_the_injected_cloc assert_eq!(completion.finish_reason(), Some("stop")); assert_eq!(seen.borrow().len(), 2, "one live delta per text fragment"); let timing = completion.client_timing().expect("timing is measured"); - assert!((timing.ttft_ms.expect("first delta") - 10.0).abs() < f64::EPSILON); - assert!((timing.mean_itl_ms.expect("two deltas") - 10.0).abs() < f64::EPSILON); - assert!((timing.e2e_ms - 30.0).abs() < f64::EPSILON); + // Rounding makes every timing a whole microsecond, so the serialized + // text is exact and short: there is no epsilon left to choose. + assert_eq!( + serde_json::to_string(timing).expect("timing serializes"), + r#"{"ttft_ms":10.0,"mean_itl_ms":10.0,"e2e_ms":30.0}"# + ); +} + +#[test] +fn read_completion_stream_rounds_timings_to_microseconds_for_the_log() { + let body = sse(&[ + text_chunk("a"), + text_chunk("b"), + text_chunk("c"), + text_chunk("d"), + json!({ "choices": [{ "index": 0, "delta": {}, "finish_reason": "stop" }] }), + json!("[DONE]"), + ]) + .replace("data: \"[DONE]\"", "data: [DONE]"); + let (head, tail) = body.split_at(body.len() / 2); + let mut source = Canned::of(&[head, tail]); + let started = Instant::now(); + // Every reading carries nanosecond noise: without rounding the logged + // text would be a long nanosecond expansion that need not survive. + // Four deltas make the mean divide by three, so its quotient is a long + // decimal only the mean's own rounding trims; with two deltas the + // divisor is one and the already-rounded gap would pass through whole. + let ticks = Cell::new(0_u32); + let now = || { + ticks.set(ticks.get() + 1); + match ticks.get() { + 1 => started + Duration::new(0, 1_234_567_891), + 2 => started + Duration::new(0, 1_600_000_000), + 3 => started + Duration::new(0, 1_900_000_000), + 4 => started + Duration::new(0, 2_234_567_891), + _ => started + Duration::new(0, 3_703_703_673), + } + }; + let completion = block_on(read_completion_stream( + &mut source, + json!({ "model": "m" }), + 1024, + |_| {}, + started, + now, + )) + .expect("a whole stream reassembles"); + let timing = completion.client_timing().expect("timing is measured"); + let text = serde_json::to_string(timing).expect("timing serializes"); + assert_eq!( + text, r#"{"ttft_ms":1234.568,"mean_itl_ms":333.333,"e2e_ms":3703.704}"#, + "each timing is a whole microsecond, so its decimal text is short" + ); + let back: ClientTiming = serde_json::from_str(&text).expect("its own text parses"); + assert_eq!(&back, timing, "every field reads back bit-for-bit"); } #[test] diff --git a/crates/promptforge/model-client/src/client/read.rs b/crates/promptforge/model-client/src/client/read.rs index 4e414f9c..92e37df1 100644 --- a/crates/promptforge/model-client/src/client/read.rs +++ b/crates/promptforge/model-client/src/client/read.rs @@ -150,9 +150,9 @@ pub async fn read_completion_stream( let client_timing = ClientTiming { ttft_ms: first_delta.map(|at| duration_ms(at.duration_since(started))), mean_itl_ms: match (first_delta, last_delta) { - (Some(first), Some(last)) if delta_chunks >= 2 => { - Some(duration_ms(last.duration_since(first)) / f64::from(delta_chunks - 1)) - } + (Some(first), Some(last)) if delta_chunks >= 2 => Some(round_to_microsecond( + duration_ms(last.duration_since(first)) / f64::from(delta_chunks - 1), + )), _ => None, }, e2e_ms: duration_ms(now().duration_since(started)), @@ -163,9 +163,19 @@ pub async fn read_completion_stream( accumulator.finish(request_body, Some(client_timing)) } -/// A duration as fractional milliseconds. +/// A duration as fractional milliseconds, rounded to a whole microsecond +/// so the text the run log stores parses back exactly on replay. fn duration_ms(duration: Duration) -> f64 { - duration.as_secs_f64() * 1000.0 + round_to_microsecond(duration.as_secs_f64() * 1000.0) +} + +/// Rounds fractional-millisecond `ms` to the nearest whole microsecond. +/// +/// A run log stores a timing as JSON text and parses that text back for +/// replay. A whole-microsecond value has a short decimal form the parser +/// reproduces exactly, so a replayed timing equals the recorded one. +fn round_to_microsecond(ms: f64) -> f64 { + (ms * 1000.0).round() / 1000.0 } #[cfg(test)] diff --git a/vibe/2026-09-22-3-replay-json-fidelity.md b/vibe/2026-09-22-3-replay-json-fidelity.md index b5f19011..d49eff34 100644 --- a/vibe/2026-09-22-3-replay-json-fidelity.md +++ b/vibe/2026-09-22-3-replay-json-fidelity.md @@ -151,7 +151,7 @@ Enable `serde_json`'s `float_roundtrip` feature on the workspace dependency in ` -### Step 2: Round ClientTiming fields to microseconds +### Step 2: Round ClientTiming fields to microseconds [completed] - Component: replay-grade log round-trip From 70033a23ffd2af3b991e31c12106dda4e2697998 Mon Sep 17 00:00:00 2001 From: Vinnie Falco Date: Tue, 22 Sep 2026 15:07:49 -0700 Subject: [PATCH 09/19] =?UTF-8?q?=EF=BB=BFPin=20log=20payload=20round-trip?= =?UTF-8?q?=20fidelity=20in=20tests?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The run log writes each record's payload as JSON text and parses it back on read, and replay compares the parsed value against the one that produced it, so a value that does not survive the round trip would make a replayed run differ from the run that ran. This adds integration tests that hold a payload to that standard across awkward floating-point values, objects built with keys in non-sorted order, arrays of mixed numbers, and a realistic assistant reply carrying its timing metrics. The stored text is checked to stay canonical as well as the value to stay identical, and the guarantee is now recorded beside the crate's other invariants. - `crates/harness/log/tests/it/fidelity.rs` is a new integration suite, wired in by the `mod fidelity;` line added to `crates/harness/log/tests/it/main.rs`. - `promptforge-api-types` is added as a dev-dependency so the suite can build a real `Event::AssistantReply` carrying `CallMetrics`. - `round_trips` appends one event to a fresh in-memory run and asserts both that the returned value equals the original and that their serialized text matches. - `AWKWARD` names a double whose shortest decimal text a plain parser reads back as a neighbouring value, so the float cases fail unless parsing is exact. - `crates/harness/log/src/lib.rs` gains a documented invariant only; the writer, reader, and stored payload shape are unchanged. Plan: vibe/2026-09-22-3-replay-json-fidelity.md --- Cargo.lock | 1 + crates/harness/log/Cargo.toml | 1 + crates/harness/log/src/lib.rs | 6 + crates/harness/log/tests/it/fidelity.rs | 173 ++++++++++++++++++++++ crates/harness/log/tests/it/main.rs | 1 + vibe/2026-09-22-3-replay-json-fidelity.md | 2 +- 6 files changed, 183 insertions(+), 1 deletion(-) create mode 100644 crates/harness/log/tests/it/fidelity.rs diff --git a/Cargo.lock b/Cargo.lock index a69df432..62423368 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2613,6 +2613,7 @@ dependencies = [ name = "harness-log" version = "0.3.0" dependencies = [ + "promptforge-api-types", "serde_json", "shared-error-source", "tempfile", diff --git a/crates/harness/log/Cargo.toml b/crates/harness/log/Cargo.toml index fb2aeaec..da01442f 100644 --- a/crates/harness/log/Cargo.toml +++ b/crates/harness/log/Cargo.toml @@ -20,6 +20,7 @@ turso.workspace = true workspace-hack.workspace = true [dev-dependencies] +promptforge-api-types.workspace = true tempfile.workspace = true tokio = { workspace = true, features = ["macros", "rt-multi-thread"] } diff --git a/crates/harness/log/src/lib.rs b/crates/harness/log/src/lib.rs index c519d26a..22ad75cc 100644 --- a/crates/harness/log/src/lib.rs +++ b/crates/harness/log/src/lib.rs @@ -20,6 +20,12 @@ //! hierarchical task path stored as text; timestamps are UTC //! milliseconds since the Unix epoch. `started_at` is the caller's; //! `at` and `ended_at` are the log's wall clock. +//! - A stored payload round-trips: `Record::payload` is written with +//! `serde_json::to_string` and read back with `from_str`, and the parsed +//! value equals the original with the same text. The workspace enables +//! `serde_json`'s `float_roundtrip` feature, and `Value::Object` orders +//! keys by `BTreeMap` rather than by insertion, so both hold. The +//! fidelity test in `tests/it/fidelity.rs` pins it. //! - Every file in this crate stays under 500 lines; split first, then //! edit. //! - Nothing in this crate spawns a tokio task directly; the harness diff --git a/crates/harness/log/tests/it/fidelity.rs b/crates/harness/log/tests/it/fidelity.rs new file mode 100644 index 00000000..97de14f7 --- /dev/null +++ b/crates/harness/log/tests/it/fidelity.rs @@ -0,0 +1,173 @@ +//! Payload fidelity: a stored payload reads back as the identical value. +//! +//! The log writes a record's payload as JSON text and parses it back on +//! read; replay compares the parsed value against the one that produced it, +//! so a float whose text does not parse exactly, or an object whose key +//! order changes, would make a replayed run differ from the run that ran. + +use harness_log::{Record, RecordFilter, RecordKind, RunLog, RunMeta}; +use promptforge_api_types::event::{Event, ReplyOrigin}; +use promptforge_api_types::ids::{ParseIdError, Provenance, TaskId}; +use promptforge_api_types::metrics::{CallMetrics, ClientTiming, LlamaTimings, Usage, VllmMetrics}; +use serde_json::{Map, Value, json}; + +/// A double whose shortest decimal text a plain parser reads back as the +/// neighbour `3.9078`: the defect exact parsing removes. +const AWKWARD: f64 = 3.907_800_000_000_000_4; + +/// Every record of a run, in loop order. +const ALL: RecordFilter = RecordFilter { + kind: None, + task: None, + last: None, +}; + +/// A run's opening row, as the harness would write it. +fn meta() -> RunMeta { + RunMeta { + session_id: "session-1".to_owned(), + agent: "chat".to_owned(), + prompt_hash: "sha256:abc".to_owned(), + seed: 3, + flags: 0, + started_at: 1_700_000_000_000, + } +} + +/// The replay key every fidelity event carries. +fn provenance() -> Result { + Ok(Provenance { + task: "0".parse::()?, + seq: 0, + }) +} + +/// Appends `payload` as the one event of a fresh run and asserts the log +/// returns the identical value and the identical text. +async fn round_trips(payload: Value) -> Result<(), Box> { + let mut log = RunLog::in_memory().await?; + let run = log.begin_run(meta()).await?; + log.append( + run, + Record { + task_id: "0".to_owned(), + task_seq: 0, + kind: RecordKind::Event, + effect_id: None, + payload: payload.clone(), + }, + ) + .await?; + + let stored = log.records(run, ALL).await?; + assert_eq!( + stored.len(), + 1, + "the run holds exactly the one event appended" + ); + let returned = &stored[0].record.payload; + assert_eq!(returned, &payload, "the value survives the round trip"); + assert_eq!( + serde_json::to_string(returned)?, + serde_json::to_string(&payload)?, + "the stored text is canonical for the value" + ); + Ok(()) +} + +#[tokio::test] +async fn awkward_floats_round_trip_through_the_log() { + // Each of these is a double whose shortest decimal text a plain + // parser does not reproduce; without exact parsing the value read + // back is a neighbour of the one written. + round_trips(json!({ + "sum": 0.1 + 0.2, + "noise": AWKWARD, + "tiny": 1e-7, + "huge": 1e21, + "max": f64::MAX, + "negative_zero": -0.0, + })) + .await + .unwrap(); +} + +#[tokio::test] +async fn nested_objects_round_trip_with_canonical_key_order() { + // Insertion order is deliberately not sorted; a `Value` object is a + // `BTreeMap`, so the stored text orders keys by byte value whatever + // the insertion order, and that canonical order must survive the read. + let mut inner = Map::new(); + inner.insert("zulu".to_owned(), json!(1)); + inner.insert("alpha".to_owned(), json!({ "yankee": 2, "bravo": 3 })); + inner.insert("mike".to_owned(), json!([{ "delta": 4, "charlie": 5 }])); + + let mut outer = Map::new(); + outer.insert("second".to_owned(), Value::Object(inner)); + outer.insert("first".to_owned(), json!(true)); + + let payload = Value::Object(outer); + assert_eq!( + serde_json::to_string(&payload).unwrap(), + r#"{"first":true,"second":{"alpha":{"bravo":3,"yankee":2},"mike":[{"charlie":5,"delta":4}],"zulu":1}}"# + ); + round_trips(payload).await.unwrap(); +} + +#[tokio::test] +async fn arrays_of_mixed_numbers_round_trip_through_the_log() { + round_trips(json!({ + "mixed": [0, -1, 1.5, 0.1 + 0.2, AWKWARD, 1e21, i64::MIN, u64::MAX], + "nested": [[1.25, 2], [3.5]], + })) + .await + .unwrap(); +} + +#[tokio::test] +async fn a_real_assistant_reply_with_metrics_round_trips_through_the_log() { + let event = Event::AssistantReply { + execution: "run-1".to_owned(), + section: "chat".to_owned(), + provenance: provenance().unwrap(), + turn: 2, + text: "hello".to_owned(), + finish_reason: Some("stop".to_owned()), + model: "llama-3".to_owned(), + origin: ReplyOrigin::Chat, + metrics: Some(CallMetrics { + usage: Some(Usage { + prompt_tokens: 7, + completion_tokens: 3, + total_tokens: 10, + cached_tokens: Some(2), + reasoning_tokens: Some(1), + }), + llama: Some(LlamaTimings { + prompt_n: 7, + prompt_ms: AWKWARD, + prompt_per_second: 560.0, + predicted_n: 3, + predicted_ms: 30.5, + predicted_per_second: 98.5, + draft_n: 4, + draft_n_accepted: 2, + }), + vllm: Some(VllmMetrics { + time_to_first_token_ms: Some(8.5), + generation_time_ms: Some(22.5), + queue_time_ms: Some(1.5), + mean_itl_ms: Some(7.5), + tokens_per_second: Some(133.5), + }), + client: Some(ClientTiming { + ttft_ms: Some(3.907_800_000_000_000_4), + mean_itl_ms: Some(8.25), + e2e_ms: 0.1 + 0.2, + }), + }), + }; + round_trips(serde_json::to_value(&event).unwrap()) + .await + .unwrap(); +} diff --git a/crates/harness/log/tests/it/main.rs b/crates/harness/log/tests/it/main.rs index 05762c1d..d73ae759 100644 --- a/crates/harness/log/tests/it/main.rs +++ b/crates/harness/log/tests/it/main.rs @@ -1,4 +1,5 @@ //! Integration tests for `harness-log`. mod append; +mod fidelity; mod read; diff --git a/vibe/2026-09-22-3-replay-json-fidelity.md b/vibe/2026-09-22-3-replay-json-fidelity.md index d49eff34..f4fc96ff 100644 --- a/vibe/2026-09-22-3-replay-json-fidelity.md +++ b/vibe/2026-09-22-3-replay-json-fidelity.md @@ -161,7 +161,7 @@ In `crates/promptforge/model-client/src/client/read.rs`, round `duration_ms` (fr -### Step 3: Pin log payload fidelity +### Step 3: Pin log payload fidelity [completed] - Component: replay-grade log round-trip From d8b19ca2a602679af79edbeb4fd7dcd07153ce93 Mon Sep 17 00:00:00 2001 From: Vinnie Falco Date: Tue, 22 Sep 2026 15:17:45 -0700 Subject: [PATCH 10/19] =?UTF-8?q?=EF=BB=BFExtract=20the=20shared=20hash-ke?= =?UTF-8?q?y=20ordering=20helper?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Move the deterministic ordering of a Lua table's hash keys into its own crate-internal module, so one classifier and one comparator define that order in a single place. The classifier reports why a key cannot be ordered, and each caller turns that reason into its own message. The enumeration path now reuses the shared helper. - `crates/promptforge/lua/src/collection-order.rs` is a new crate-internal module; `collection.rs` declares it, re-exports `SortKey`, `sort_key`, and `KeyError`, and the ordering tests move to a sibling test file. - `compare_integer_float` moves into the new module unchanged, keeping an integer ordered exactly against a nearby float without rounding the integer. - `sort_key` classifies one Lua value into a sort position and a label, and keeps an integer key an integer rather than a float. - `KeyError` names three failures, a non-finite number, a non-UTF-8 string, and an unsortable type, and `collection_members` maps each onto fanout's own message. - `crates/promptforge/lua/src/collection.rs` keeps every extracted item crate-internal, so no public API is added. Design: new pure-function @ crates/promptforge/lua/src/collection-order.rs::sort_key deps: Value Design: replaces pure-function @ crates/promptforge/lua/src/collection-order.rs::compare_integer_float deps: f64,i64 was: crates/promptforge/lua/src/collection.rs::compare_integer_float Plan: vibe/2026-09-22-3-replay-json-fidelity.md --- .../lua/src/collection-order-tests.rs | 64 ++++++++ .../promptforge/lua/src/collection-order.rs | 143 +++++++++++++++++ crates/promptforge/lua/src/collection.rs | 144 ++---------------- vibe/2026-09-22-3-replay-json-fidelity.md | 2 +- 4 files changed, 223 insertions(+), 130 deletions(-) create mode 100644 crates/promptforge/lua/src/collection-order-tests.rs create mode 100644 crates/promptforge/lua/src/collection-order.rs diff --git a/crates/promptforge/lua/src/collection-order-tests.rs b/crates/promptforge/lua/src/collection-order-tests.rs new file mode 100644 index 00000000..ecbae7ca --- /dev/null +++ b/crates/promptforge/lua/src/collection-order-tests.rs @@ -0,0 +1,64 @@ +//! Tests for the shared hash-key ordering helper. + +use std::cmp::Ordering; + +use mlua::{Lua, Value}; + +use super::{KeyError, SortKey, compare_integer_float, sort_key}; + +#[test] +fn compare_integer_float_orders_without_rounding_the_integer() { + assert_eq!(compare_integer_float(2, 2.5), Ordering::Less); + assert_eq!(compare_integer_float(3, 2.5), Ordering::Greater); + assert_eq!(compare_integer_float(2, 2.0), Ordering::Equal); + assert_eq!(compare_integer_float(-1, -0.5), Ordering::Less); + assert_eq!(compare_integer_float(0, -0.5), Ordering::Greater); + assert_eq!(compare_integer_float(i64::MAX, 1e300), Ordering::Less); + assert_eq!(compare_integer_float(i64::MIN, -1e300), Ordering::Greater); + // 2^63 as a float is one past i64::MAX, so the largest integer is + // still below it; -2^63 is exactly i64::MIN. + assert_eq!( + compare_integer_float(i64::MAX, 9_223_372_036_854_775_808.0), + Ordering::Less + ); + assert_eq!( + compare_integer_float(i64::MIN, -9_223_372_036_854_775_808.0), + Ordering::Equal + ); +} + +#[test] +fn sort_key_classifies_scalars_and_rejects_unsortable_keys() { + let lua = Lua::new(); + + let (key, label) = sort_key(&Value::Boolean(false)).expect("a boolean classifies"); + assert!(matches!(key, SortKey::Bool(false))); + assert_eq!(label, "false"); + + // An integer stays an integer: a float classification would tie two + // distinct integers past 2^53 and hand their order back to `pairs`. + let (key, label) = sort_key(&Value::Integer(i64::MAX)).expect("an integer classifies"); + assert!(matches!(key, SortKey::Integer(i64::MAX))); + assert_eq!(label, i64::MAX.to_string()); + + let (key, label) = sort_key(&Value::Number(2.5)).expect("a float classifies"); + assert!(matches!(key, SortKey::Float(value) if value.total_cmp(&2.5).is_eq())); + assert_eq!(label, "2.5"); + + let text = lua.create_string("alpha").expect("a string creates"); + let (key, label) = sort_key(&Value::String(text)).expect("a string classifies"); + assert!(matches!(key, SortKey::Text(bytes) if bytes == b"alpha")); + assert_eq!(label, "alpha"); + + // A non-finite number has no ordered position, so it is rejected + // rather than silently ordered by a NaN or infinity comparison. + let non_finite = + sort_key(&Value::Number(f64::INFINITY)).expect_err("a non-finite number fails"); + assert!(matches!(non_finite, KeyError::NotFinite)); + + // A table key has no cross-type rank, so it is rejected rather than + // dropped or ordered by an invented comparison. + let table = lua.create_table().expect("a table creates"); + let unsortable = sort_key(&Value::Table(table)).expect_err("a non-scalar key fails"); + assert!(matches!(unsortable, KeyError::Unsortable("table"))); +} diff --git a/crates/promptforge/lua/src/collection-order.rs b/crates/promptforge/lua/src/collection-order.rs new file mode 100644 index 00000000..612ccaca --- /dev/null +++ b/crates/promptforge/lua/src/collection-order.rs @@ -0,0 +1,143 @@ +//! The shared ordering of a Lua table's hash keys. +//! +//! Lua leaves a table's hash traversal order unspecified, so every path that +//! turns a table into an ordered sequence imposes the same order: booleans +//! first (`false` before `true`), then numbers by value, then strings +//! bytewise. [`sort_key`] classifies one Lua key into its [`SortKey`]; +//! [`SortKey::compare`] orders two of them. Fanout's member enumeration and +//! the deterministic `pairs`/`next` installer share both, so a table's order +//! is a function of its contents wherever it is read. + +use std::cmp::Ordering; + +use mlua::Value; + +/// A hash key's sort position: booleans first (`false` before `true`), then +/// numbers by value, then strings bytewise. The ranks keep mixed-type keys +/// totally ordered without inventing a cross-type comparison. An integer +/// key stays an `i64` so two distinct integers past 2^53 never compare +/// equal (which would leave their order to `pairs`, the nondeterminism the +/// sort exists to remove); only a mixed integer/float pair converts. +#[derive(Debug)] +pub(crate) enum SortKey { + Bool(bool), + Integer(i64), + Float(f64), + Text(Vec), +} + +impl SortKey { + /// The type's ordering rank: booleans, then numbers, then strings. + fn rank(&self) -> u8 { + match self { + SortKey::Bool(_) => 0, + SortKey::Integer(_) | SortKey::Float(_) => 1, + SortKey::Text(_) => 2, + } + } + + /// Orders two positions of any type: same types compare within their + /// type, a mixed integer and float compares exactly, and a mixed type + /// falls back to its rank. + pub(crate) fn compare(&self, other: &SortKey) -> Ordering { + match (self, other) { + (SortKey::Bool(left), SortKey::Bool(right)) => left.cmp(right), + (SortKey::Integer(left), SortKey::Integer(right)) => left.cmp(right), + (SortKey::Float(left), SortKey::Float(right)) => left.total_cmp(right), + (SortKey::Integer(integer), SortKey::Float(float)) => { + compare_integer_float(*integer, *float) + } + (SortKey::Float(float), SortKey::Integer(integer)) => { + compare_integer_float(*integer, *float).reverse() + } + (SortKey::Text(left), SortKey::Text(right)) => left.cmp(right), + _ => self.rank().cmp(&other.rank()), + } + } +} + +/// Orders an integer key against a finite float key exactly: the float is +/// compared to the integer's neighborhood without rounding the integer, +/// so an integer past 2^53 still sorts on the correct side of a nearby +/// float. A float outside `i64`'s range is beyond every integer; a float +/// inside it is truncated, the integer parts are compared, and a tie is +/// broken by the float's fractional part (an exact integer-valued float +/// ties with its integer). +fn compare_integer_float(integer: i64, float: f64) -> Ordering { + /// 2^63: one past `i64::MAX`, exactly representable, so a float at or + /// beyond it is greater than every integer. + const ABOVE_MAX: f64 = 9_223_372_036_854_775_808.0; + /// -2^63: exactly `i64::MIN`, so a float below it is less than every + /// integer. + const MIN: f64 = -9_223_372_036_854_775_808.0; + if float >= ABOVE_MAX { + return Ordering::Less; + } + if float < MIN { + return Ordering::Greater; + } + // In range and finite: the truncation is exact for the integer part. + #[expect( + clippy::cast_possible_truncation, + reason = "the float is inside i64's range and its fractional part is compared separately" + )] + let truncated = float.trunc() as i64; + match integer.cmp(&truncated) { + Ordering::Equal => { + // The integer equals the float's integer part, so it sits below + // a float with a positive fraction and above one with a + // negative fraction. + let fraction = float - float.trunc(); + if fraction > 0.0 { + Ordering::Less + } else if fraction < 0.0 { + Ordering::Greater + } else { + Ordering::Equal + } + } + ordering => ordering, + } +} + +/// Why a Lua value cannot be a sortable hash key. +#[derive(Debug)] +pub(crate) enum KeyError { + /// A number key that is NaN or infinite: it has no ordered position. + NotFinite, + /// A string key whose bytes are not valid UTF-8, so it has no label. + NotUtf8(mlua::Error), + /// A key whose Lua type is not string, number, or boolean. + Unsortable(&'static str), +} + +/// Classifies a Lua value as a sortable hash key: its sort position plus the +/// label naming it (a string's text, any other scalar's rendering). +/// +/// # Errors +/// Returns [`KeyError`] when the value has no ordered position: a non-scalar +/// key, a string whose bytes are not valid UTF-8, or a number that is not +/// finite. Each caller maps the failure onto its own diagnostic, so this +/// helper names no feature. +pub(crate) fn sort_key(key: &Value) -> Result<(SortKey, String), KeyError> { + match key { + Value::String(s) => { + let label = s.to_str().map_err(KeyError::NotUtf8)?.to_owned(); + Ok((SortKey::Text(s.as_bytes().to_vec()), label)) + } + Value::Integer(i) => Ok((SortKey::Integer(*i), i.to_string())), + Value::Number(n) => { + if n.is_finite() { + Ok((SortKey::Float(*n), n.to_string())) + } else { + Err(KeyError::NotFinite) + } + } + Value::Boolean(b) => Ok((SortKey::Bool(*b), b.to_string())), + other => Err(KeyError::Unsortable(other.type_name())), + } +} + +#[cfg(test)] +#[path = "collection-order-tests.rs"] +mod tests; diff --git a/crates/promptforge/lua/src/collection.rs b/crates/promptforge/lua/src/collection.rs index 1263af4d..bccb9c15 100644 --- a/crates/promptforge/lua/src/collection.rs +++ b/crates/promptforge/lua/src/collection.rs @@ -9,94 +9,14 @@ //! array member arrives as the arm's `item` value as itself; a hash member //! arrives as the pair table (`item.key` / `item.value`). -use std::cmp::Ordering; - use mlua::{Lua, LuaSerdeExt, Table, Value}; use crate::error::{Error, Result}; -/// A hash key's sort position: booleans first (`false` before `true`), then -/// numbers by value, then strings bytewise. The ranks keep mixed-type keys -/// totally ordered without inventing a cross-type comparison. An integer -/// key stays an `i64` so two distinct integers past 2^53 never compare -/// equal (which would leave their order to `pairs`, the nondeterminism the -/// sort exists to remove); only a mixed integer/float pair converts. -enum SortKey { - Bool(bool), - Integer(i64), - Float(f64), - Text(Vec), -} +#[path = "collection-order.rs"] +mod order; -impl SortKey { - fn rank(&self) -> u8 { - match self { - SortKey::Bool(_) => 0, - SortKey::Integer(_) | SortKey::Float(_) => 1, - SortKey::Text(_) => 2, - } - } - - fn compare(&self, other: &SortKey) -> Ordering { - match (self, other) { - (SortKey::Bool(left), SortKey::Bool(right)) => left.cmp(right), - (SortKey::Integer(left), SortKey::Integer(right)) => left.cmp(right), - (SortKey::Float(left), SortKey::Float(right)) => left.total_cmp(right), - (SortKey::Integer(integer), SortKey::Float(float)) => { - compare_integer_float(*integer, *float) - } - (SortKey::Float(float), SortKey::Integer(integer)) => { - compare_integer_float(*integer, *float).reverse() - } - (SortKey::Text(left), SortKey::Text(right)) => left.cmp(right), - _ => self.rank().cmp(&other.rank()), - } - } -} - -/// Orders an integer key against a finite float key exactly: the float is -/// compared to the integer's neighborhood without rounding the integer, -/// so an integer past 2^53 still sorts on the correct side of a nearby -/// float. A float outside `i64`'s range is beyond every integer; a float -/// inside it is truncated, the integer parts are compared, and a tie is -/// broken by the float's fractional part (an exact integer-valued float -/// ties with its integer). -fn compare_integer_float(integer: i64, float: f64) -> Ordering { - /// 2^63: one past `i64::MAX`, exactly representable, so a float at or - /// beyond it is greater than every integer. - const ABOVE_MAX: f64 = 9_223_372_036_854_775_808.0; - /// -2^63: exactly `i64::MIN`, so a float below it is less than every - /// integer. - const MIN: f64 = -9_223_372_036_854_775_808.0; - if float >= ABOVE_MAX { - return Ordering::Less; - } - if float < MIN { - return Ordering::Greater; - } - // In range and finite: the truncation is exact for the integer part. - #[expect( - clippy::cast_possible_truncation, - reason = "the float is inside i64's range and its fractional part is compared separately" - )] - let truncated = float.trunc() as i64; - match integer.cmp(&truncated) { - Ordering::Equal => { - // The integer equals the float's integer part, so it sits below - // a float with a positive fraction and above one with a - // negative fraction. - let fraction = float - float.trunc(); - if fraction > 0.0 { - Ordering::Less - } else if fraction < 0.0 { - Ordering::Greater - } else { - Ordering::Equal - } - } - ordering => ordering, - } -} +pub(crate) use order::{KeyError, SortKey, sort_key}; /// Enumerates fanout's collection argument as the sequence of members the /// shim spawns arms over: the array part (`1..=#t`) in order, then the hash @@ -130,33 +50,20 @@ pub(crate) fn collection_members(lua: &Lua, collection: &Value) -> Result { continue; } - // Each scalar key yields its sort position and its diagnostic label - // in one match; non-scalar keys are rejected here, so no later code - // path can meet one. - let (sort_key, key_label) = match &key { - Value::String(s) => { - let text = s.to_str().map_err(Error::lua)?; - (SortKey::Text(s.as_bytes().to_vec()), text.to_owned()) - } - Value::Integer(i) => (SortKey::Integer(*i), i.to_string()), - Value::Number(n) => { - if !n.is_finite() { - return Err(Error::Lua( - "fanout collection key is not a finite number".to_owned(), - )); - } - (SortKey::Float(*n), n.to_string()) + // The shared classifier yields each scalar key's sort position and + // its diagnostic label; the failure is mapped to fanout's own message + // here, so a non-scalar key is rejected before any later code path. + let (position, key_label) = sort_key(&key).map_err(|error| match error { + KeyError::NotFinite => { + Error::Lua("fanout collection key is not a finite number".to_owned()) } - Value::Boolean(b) => (SortKey::Bool(*b), b.to_string()), - other => { - return Err(Error::Lua(format!( - "fanout collection key must be a string, number, or boolean, got {}", - other.type_name() - ))); - } - }; + KeyError::NotUtf8(source) => Error::lua(source), + KeyError::Unsortable(type_name) => Error::Lua(format!( + "fanout collection key must be a string, number, or boolean, got {type_name}" + )), + })?; check_member(&member, &key_label)?; - pairs.push((sort_key, key, member)); + pairs.push((position, key, member)); } pairs.sort_by(|left, right| left.0.compare(&right.0)); for (position, (_, key, member)) in pairs.into_iter().enumerate() { @@ -321,27 +228,6 @@ mod tests { ); } - #[test] - fn compare_integer_float_orders_without_rounding_the_integer() { - assert_eq!(compare_integer_float(2, 2.5), Ordering::Less); - assert_eq!(compare_integer_float(3, 2.5), Ordering::Greater); - assert_eq!(compare_integer_float(2, 2.0), Ordering::Equal); - assert_eq!(compare_integer_float(-1, -0.5), Ordering::Less); - assert_eq!(compare_integer_float(0, -0.5), Ordering::Greater); - assert_eq!(compare_integer_float(i64::MAX, 1e300), Ordering::Less); - assert_eq!(compare_integer_float(i64::MIN, -1e300), Ordering::Greater); - // 2^63 as a float is one past i64::MAX, so the largest integer is - // still below it; -2^63 is exactly i64::MIN. - assert_eq!( - compare_integer_float(i64::MAX, 9_223_372_036_854_775_808.0), - Ordering::Less - ); - assert_eq!( - compare_integer_float(i64::MIN, -9_223_372_036_854_775_808.0), - Ordering::Equal - ); - } - #[test] fn collection_members_emits_the_array_part_before_the_hash_part() { let lua = mlua::Lua::new(); diff --git a/vibe/2026-09-22-3-replay-json-fidelity.md b/vibe/2026-09-22-3-replay-json-fidelity.md index f4fc96ff..5721e295 100644 --- a/vibe/2026-09-22-3-replay-json-fidelity.md +++ b/vibe/2026-09-22-3-replay-json-fidelity.md @@ -171,7 +171,7 @@ Add `crates/harness/log/tests/it/fidelity.rs` covering awkward floats, nested ob -### Step 4: Extract the shared ordering helper +### Step 4: Extract the shared ordering helper [completed] - Component: deterministic Lua iteration From 201acd641d93e3aa78b235b6347babf891df5a04 Mon Sep 17 00:00:00 2001 From: Vinnie Falco Date: Tue, 22 Sep 2026 15:24:52 -0700 Subject: [PATCH 11/19] =?UTF-8?q?=EF=BB=BFInstall=20deterministic=20sorted?= =?UTF-8?q?=20pairs=20and=20next?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Lua leaves a table's hash traversal order unspecified, so two runs can visit the same table differently. This change replaces a section runtime's iterator and pair-traversal functions with one deterministic walk: the array part in index order, then the remaining keys by a fixed cross-type order, with a metamethod still taking precedence. A walk that loses its current key partway through resumes at the next live key instead of stopping or repeating. - `ordered_keys` fixes the walk order: the array part (`1..=#t`) in index order, then scalar hash keys by `SortKey`, then any non-scalar keys. - `install_deterministic_iteration` replaces the `next` and `pairs` globals on the supplied VM and surfaces an error when either global cannot be set. - `pairs` invokes a table's `__pairs` metamethod when one is present, as stock Lua does, and otherwise returns the installed `next`, the table, and a nil cursor. - `next` is stateless: `ordered_keys` rebuilds the live key sequence on every call, so a key cleared to nil between steps is skipped and the walk continues. - `next_index` resumes at the first key sorting strictly after a previous key that is no longer present, so clearing the current key does not end the traversal. - `iteration` has no production caller in this change; `lib.rs` gates it with `#[cfg_attr(not(test), expect(dead_code))]`, so a section VM still iterates with the stock functions. Design: new pure-function @ crates/promptforge/lua/src/iteration.rs::ordered_keys deps: Table Design: new pure-function @ crates/promptforge/lua/src/iteration.rs::next_index deps: Value Plan: vibe/2026-09-22-3-replay-json-fidelity.md --- crates/promptforge/lua/src/iteration-tests.rs | 135 +++++++++++++++ crates/promptforge/lua/src/iteration.rs | 161 ++++++++++++++++++ crates/promptforge/lua/src/lib.rs | 2 + vibe/2026-09-22-3-replay-json-fidelity.md | 2 +- 4 files changed, 299 insertions(+), 1 deletion(-) create mode 100644 crates/promptforge/lua/src/iteration-tests.rs create mode 100644 crates/promptforge/lua/src/iteration.rs diff --git a/crates/promptforge/lua/src/iteration-tests.rs b/crates/promptforge/lua/src/iteration-tests.rs new file mode 100644 index 00000000..9c00ab42 --- /dev/null +++ b/crates/promptforge/lua/src/iteration-tests.rs @@ -0,0 +1,135 @@ +//! Tests for the deterministic `pairs`/`next` installer. + +use mlua::{Lua, LuaSerdeExt, Value}; +use serde_json::json; + +use super::install_deterministic_iteration; + +/// A VM with the deterministic iterator installed, as a section VM has it. +fn vm() -> Lua { + let lua = Lua::new(); + install_deterministic_iteration(&lua).expect("the installer runs"); + lua +} + +/// Evaluates a chunk that returns a sequence table and reads it back as JSON. +fn sequence(lua: &Lua, source: &str) -> Vec { + let value: Value = lua.load(source).eval().expect("the chunk evaluates"); + lua.from_value(value).expect("the result is JSON data") +} + +#[test] +fn pairs_visits_string_keys_in_byte_order() { + // Two insertion orders produce the same key sequence: the order is a + // function of the keys, not of the table's hash state. + let lua = vm(); + let source = "local out = {}; for k in pairs({zeta=1, alpha=2, mid=3, beta=4}) \ + do out[#out+1] = k end; return out"; + assert_eq!( + sequence(&lua, source), + vec![json!("alpha"), json!("beta"), json!("mid"), json!("zeta")] + ); + let source = "local out = {}; for k in pairs({beta=4, mid=3, alpha=2, zeta=1}) \ + do out[#out+1] = k end; return out"; + assert_eq!( + sequence(&lua, source), + vec![json!("alpha"), json!("beta"), json!("mid"), json!("zeta")] + ); +} + +#[test] +fn pairs_visits_the_array_part_before_the_hash_part() { + let lua = vm(); + let source = "local out = {}; for k in pairs({10, 20, 30, extra='x', another='y'}) \ + do out[#out+1] = k end; return out"; + assert_eq!( + sequence(&lua, source), + vec![ + json!(1), + json!(2), + json!(3), + json!("another"), + json!("extra") + ] + ); +} + +#[test] +fn pairs_orders_mixed_key_types_booleans_then_numbers_then_strings() { + let lua = vm(); + let source = "local out = {}; \ + for k in pairs({[true]='t', [7]='seven', b='bee', [false]='f', [2.5]='half', a='ay'}) \ + do out[#out+1] = k end; return out"; + assert_eq!( + sequence(&lua, source), + vec![ + json!(false), + json!(true), + json!(2.5), + json!(7), + json!("a"), + json!("b"), + ] + ); +} + +#[test] +fn pairs_honors_a_pairs_metamethod() { + // The table is empty, so only the metamethod can yield the two pairs; a + // stock `pairs` would return nothing. + let lua = vm(); + let source = "local t = setmetatable({}, {__pairs = function() \ + local i = 0 \ + return function() i = i + 1; if i <= 2 then return i, i * 10 end end \ + end}) \ + local out = {} \ + for k, v in pairs(t) do out[#out+1] = k .. ':' .. v end \ + return out"; + assert_eq!(sequence(&lua, source), vec![json!("1:10"), json!("2:20")]); +} + +#[test] +fn next_is_nil_for_an_empty_table() { + let lua = vm(); + let source = "local t = {}; return {next(t) == nil, next(t, nil) == nil}"; + assert_eq!(sequence(&lua, source), vec![json!(true), json!(true)]); +} + +#[test] +fn next_returns_the_first_pair_of_a_non_empty_table() { + let lua = vm(); + let source = "local k, v = next({only=1}); return {k == 'only', v == 1}"; + assert_eq!(sequence(&lua, source), vec![json!(true), json!(true)]); +} + +#[test] +fn pairs_skips_a_key_cleared_before_it_is_visited() { + let lua = vm(); + let source = "local t = {a=1, b=2, c=3} \ + local out = {} \ + for k in pairs(t) do \ + out[#out+1] = k \ + if k == 'a' then t.b = nil end \ + end \ + return out"; + assert_eq!(sequence(&lua, source), vec![json!("a"), json!("c")]); +} + +#[test] +fn pairs_skips_the_key_cleared_by_the_current_step() { + // Clearing the current key leaves the previous-key cursor pointing at a + // key no longer in the table; the walk resumes at the next live key + // instead of erroring or dropping the rest of the traversal. + let lua = vm(); + let source = "local t = {a=1, b=2, c=3} \ + local out = {} \ + for k in pairs(t) do \ + out[#out+1] = k \ + t[k] = nil \ + end \ + return out"; + assert_eq!( + sequence(&lua, source), + vec![json!("a"), json!("b"), json!("c")] + ); +} diff --git a/crates/promptforge/lua/src/iteration.rs b/crates/promptforge/lua/src/iteration.rs new file mode 100644 index 00000000..e5781e41 --- /dev/null +++ b/crates/promptforge/lua/src/iteration.rs @@ -0,0 +1,161 @@ +//! Deterministic `pairs`/`next` for section VMs. +//! +//! Lua leaves a table's hash traversal order unspecified, so two runs (or two +//! fresh VMs) can visit the same table differently. [`install_deterministic_iteration`] +//! replaces the `pairs` and `next` globals with one deterministic walk: the +//! array part (`1..=#t`) first in index order, then the hash part by the shared +//! [`SortKey`](crate::collection::SortKey) order (booleans, then numbers, then +//! strings). A `__pairs` metamethod still wins, exactly as in stock Lua. +//! +//! `next` is stateless: each call rebuilds the ordered key sequence from the +//! live table, so a key whose value became `nil` mid-traversal is skipped +//! rather than visited with a nil value, and traversal always advances to a +//! key strictly after the previous one. Rebuilding costs `O(n log n)` per +//! step; section tables are small and the determinism is the point. The +//! ordering covers string, number, and boolean keys, the only keys a payload +//! the run log stores can carry. A non-scalar key (table, function, userdata, +//! or thread) is still visited rather than dropped, but it sits after every +//! scalar key in the order Lua's own `next` produced it: such a key has no +//! cross-process position, and it cannot back a logged value. + +use std::cmp::Ordering; + +use mlua::{Function, Lua, MultiValue, Table, Value}; + +use crate::collection::{SortKey, sort_key}; +use crate::error::{Error, Result}; + +/// Installs the deterministic `next` and `pairs` globals, replacing the base +/// library's hash-order versions. +/// +/// # Errors +/// Returns [`Error::Lua`] if either global cannot be created or installed. +pub(crate) fn install_deterministic_iteration(lua: &Lua) -> Result<()> { + let globals = lua.globals(); + let next_fn = lua + .create_function(deterministic_next) + .map_err(Error::lua)?; + globals.raw_set("next", &next_fn).map_err(Error::lua)?; + let pairs_fn = lua + .create_function(move |_, value: Value| pairs(value, &next_fn)) + .map_err(Error::lua)?; + globals.raw_set("pairs", pairs_fn).map_err(Error::lua) +} + +/// The `next(table, key)` replacement: the key strictly after `key` in the +/// sorted order, with its value, or `(nil, nil)` at the end. +fn deterministic_next( + _lua: &Lua, + (table, previous): (Table, Value), +) -> mlua::Result<(Value, Value)> { + let keys = ordered_keys(&table).map_err(mlua::Error::external)?; + let Some(index) = next_index(&keys, &previous) else { + return Ok((Value::Nil, Value::Nil)); + }; + let key = keys[index].clone(); + let value: Value = table.raw_get(&key)?; + Ok((key, value)) +} + +/// The `pairs(table)` replacement: a `__pairs` metamethod's results when one +/// is present, otherwise `(next, table, nil)` over the installed `next`. +fn pairs(value: Value, next_fn: &Function) -> mlua::Result { + let table = match &value { + Value::Table(table) => table.clone(), + other => { + return Err(mlua::Error::runtime(format!( + "bad argument #1 to 'pairs' (table expected, got {})", + other.type_name() + ))); + } + }; + if let Some(metatable) = table.metatable() { + match metatable.raw_get::("__pairs")? { + Value::Nil => {} + Value::Function(callable) => return callable.call::(value), + other => { + return Err(mlua::Error::runtime(format!( + "attempt to call a {} value (metamethod '__pairs')", + other.type_name() + ))); + } + } + } + let mut iterator = MultiValue::new(); + iterator.push_back(Value::Function(next_fn.clone())); + iterator.push_back(value); + iterator.push_back(Value::Nil); + Ok(iterator) +} + +/// The table's live keys in deterministic walk order: the array part +/// (`1..=#table`) in index order, then the hash part's scalar keys by +/// [`SortKey`], then any non-scalar keys. +fn ordered_keys(table: &Table) -> mlua::Result> { + let border = table.raw_len(); + let mut keys: Vec = Vec::new(); + for index in 1..=border { + let value: Value = table.raw_get(index)?; + if matches!(value, Value::Nil) { + continue; + } + let index = i64::try_from(index).map_err(|_| { + mlua::Error::runtime("table index exceeds the integer range".to_owned()) + })?; + keys.push(Value::Integer(index)); + } + let mut hashed: Vec<(SortKey, Value)> = Vec::new(); + let mut unordered: Vec = Vec::new(); + for pair in table.pairs::() { + let (key, value) = pair?; + if matches!(value, Value::Nil) { + continue; + } + if let Value::Integer(index) = &key + && usize::try_from(*index).is_ok_and(|index| (1..=border).contains(&index)) + { + continue; + } + match sort_key(&key) { + Ok((position, _)) => hashed.push((position, key)), + Err(_) => unordered.push(key), + } + } + hashed.sort_by(|left, right| left.0.compare(&right.0)); + keys.extend(hashed.into_iter().map(|(_, key)| key)); + keys.extend(unordered); + Ok(keys) +} + +/// The index in `keys` to return for a `next` call with `previous` as the +/// last key. An exact match advances one slot; a key whose value was cleared +/// (`previous` absent) resumes at the first key that sorts strictly after it, +/// so the cleared key is skipped rather than revisited. +fn next_index(keys: &[Value], previous: &Value) -> Option { + if matches!(previous, Value::Nil) { + return (!keys.is_empty()).then_some(0); + } + if let Some(index) = keys.iter().position(|key| key == previous) { + return (index + 1 < keys.len()).then_some(index + 1); + } + let Ok((position, _)) = sort_key(previous) else { + return None; + }; + let mut unordered = None; + for (index, key) in keys.iter().enumerate() { + match sort_key(key) { + Ok((candidate, _)) if candidate.compare(&position) == Ordering::Greater => { + return Some(index); + } + Err(_) => { + unordered.get_or_insert(index); + } + Ok(_) => {} + } + } + unordered +} + +#[cfg(test)] +#[path = "iteration-tests.rs"] +mod tests; diff --git a/crates/promptforge/lua/src/lib.rs b/crates/promptforge/lua/src/lib.rs index b3e226e9..10e87efc 100644 --- a/crates/promptforge/lua/src/lib.rs +++ b/crates/promptforge/lua/src/lib.rs @@ -95,6 +95,8 @@ pub use error_value::{ErrorKind, ErrorValue, Raised, error_table}; mod hardening; pub(crate) use hardening::{InstructionBudget, harden, install_instruction_budget, scalar_return}; mod coro; +#[cfg_attr(not(test), expect(dead_code))] +mod iteration; pub(crate) use coro::{block_guard, install_shim_prelude, take_failure}; mod dispatch; mod sys; diff --git a/vibe/2026-09-22-3-replay-json-fidelity.md b/vibe/2026-09-22-3-replay-json-fidelity.md index 5721e295..256834a6 100644 --- a/vibe/2026-09-22-3-replay-json-fidelity.md +++ b/vibe/2026-09-22-3-replay-json-fidelity.md @@ -181,7 +181,7 @@ Extract `SortKey` and a `sort_key` helper from `crates/promptforge/lua/src/colle -### Step 5: Install sorted pairs and next +### Step 5: Install sorted pairs and next [completed] - Component: deterministic Lua iteration From 1c2384ad211f095b2a514f5fab2d63ba754d3414 Mon Sep 17 00:00:00 2001 From: Vinnie Falco Date: Tue, 22 Sep 2026 15:33:07 -0700 Subject: [PATCH 12/19] =?UTF-8?q?=EF=BB=BFActivate=20deterministic=20itera?= =?UTF-8?q?tion=20in=20the=20section=20VM?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every section virtual machine now installs a deterministic table walk as it is constructed, so two runs over the same table visit its keys in the same order instead of following Lua's unspecified hash order. The installation replaces the machine's iterator and pair-traversal functions during construction, positioned after the sandbox is applied. A new test proves that two freshly built machines over one table produce the identical key sequence, which is the stability replay comparison depends on. - `install_deterministic_iteration` is now called from the section constructor after `harden` and before `install_untrusted`, so every section virtual machine walks tables in the sorted order rather than Lua's hash order. - `crates/promptforge/lua/src/lib.rs` removes the temporary dead-code expectation that stood in for this caller and re-exports the installer at the crate root. - `two_fresh_section_vms_yield_the_same_key_order` builds two fresh machines over one five-key table and asserts both return the same sorted key sequence. Design: new surface-growth @ crates/promptforge/lua/src/vm.rs::SectionVm::new Design: extends facade @ crates/promptforge/lua/src/lib.rs Repairs: deterministic iteration order @ crates/promptforge/lua/src/vm.rs::SectionVm::new - two fresh section virtual machines visited one table in different key orders Plan: vibe/2026-09-22-3-replay-json-fidelity.md --- crates/promptforge/lua/src/lib.rs | 2 +- crates/promptforge/lua/src/tests.rs | 28 +++++++++++++++++++++++ crates/promptforge/lua/src/vm.rs | 12 ++++++---- vibe/2026-09-22-3-replay-json-fidelity.md | 2 +- 4 files changed, 38 insertions(+), 6 deletions(-) diff --git a/crates/promptforge/lua/src/lib.rs b/crates/promptforge/lua/src/lib.rs index 10e87efc..744f9e60 100644 --- a/crates/promptforge/lua/src/lib.rs +++ b/crates/promptforge/lua/src/lib.rs @@ -95,9 +95,9 @@ pub use error_value::{ErrorKind, ErrorValue, Raised, error_table}; mod hardening; pub(crate) use hardening::{InstructionBudget, harden, install_instruction_budget, scalar_return}; mod coro; -#[cfg_attr(not(test), expect(dead_code))] mod iteration; pub(crate) use coro::{block_guard, install_shim_prelude, take_failure}; +pub(crate) use iteration::install_deterministic_iteration; mod dispatch; mod sys; pub(crate) use sys::{guarded_var, seal_sys, var_snapshot_table, var_to_json}; diff --git a/crates/promptforge/lua/src/tests.rs b/crates/promptforge/lua/src/tests.rs index 4beaceb2..18e44531 100644 --- a/crates/promptforge/lua/src/tests.rs +++ b/crates/promptforge/lua/src/tests.rs @@ -1055,6 +1055,34 @@ fn section_vm_is_send() { assert_send::(); } +#[test] +fn two_fresh_section_vms_yield_the_same_key_order() { + // A section VM inherits Lua's per-state hash traversal, so two fresh VMs + // can walk one table differently and neither walk is the shared order. + // The deterministic installer in `SectionVm::new` pins both to the same + // sorted sequence, so the two walks and the expected order all agree. + let source = "local out = {} \ + for k in pairs({zeta=1, alpha=2, mid=3, beta=4, omega=5}) \ + do out[#out+1] = k end \ + return out"; + let order = |section: &str| -> Vec { + let mut vm = SectionVm::new(&test_nonce(), &null_emitter(), section) + .expect("section VM construction cannot fail"); + vm.inject_host("", &json!({}), &fresh_access()) + .expect("host values must inject"); + let keys: Vec = vm + .lua() + .load(source) + .eval() + .expect("pairs collects the string keys"); + vm.teardown(&null_emitter(), section); + keys + }; + let expected = vec!["alpha", "beta", "mid", "omega", "zeta"]; + assert_eq!(order("First"), expected); + assert_eq!(order("Second"), expected); +} + #[test] fn section_vm_preserves_one_environment_across_all_phases() { // The shared library replays as the section's first chunk with the full diff --git a/crates/promptforge/lua/src/vm.rs b/crates/promptforge/lua/src/vm.rs index 756d693d..7099dd66 100644 --- a/crates/promptforge/lua/src/vm.rs +++ b/crates/promptforge/lua/src/vm.rs @@ -7,8 +7,8 @@ use super::{ LuaToolHandle, ModelBinding, ModelRuntime, ModelSet, ModelView, ModelsInferHook, MultiValue, Mutex, Ordering, ProseState, Result, StdLib, Thread, ThreadStatus, ToolBinding, ToolCallCounts, ToolRuntime, ToolSet, Value, block_guard, guarded_var, harden, install_compactors, - install_instruction_budget, install_log, install_messages, install_models, - install_shim_prelude, install_store_table, + install_deterministic_iteration, install_instruction_budget, install_log, install_messages, + install_models, install_shim_prelude, install_store_table, install_tool_call_counts as install_tool_call_counts_impl, install_tools, install_untrusted, lifecycle, log_byte_budget, resolve_section_target, scalar_return, seal_sys, take_failure, var_to_json, @@ -212,8 +212,9 @@ impl LocalTools { impl SectionVm { /// Creates a hardened section VM. /// - /// Construction installs only the sandbox, the default resource ceilings, - /// the instruction hook, and `untrusted` (wrapping under the run's + /// Construction installs only the sandbox, the deterministic + /// `pairs`/`next` walk, the default resource ceilings, the instruction + /// hook, and `untrusted` (wrapping under the run's /// `nonce`). Everything else - the run's /// limits, the host values, the persistent host APIs, the control /// globals, the shared-library replay, and the captured alias globals - @@ -275,6 +276,9 @@ impl SectionVm { if let Err(error) = harden(&vm.lua) { return vm.construction_failed(error, emitter, section); } + if let Err(error) = install_deterministic_iteration(&vm.lua) { + return vm.construction_failed(error, emitter, section); + } if let Err(error) = install_untrusted(&vm.lua, nonce) { return vm.construction_failed(error, emitter, section); } diff --git a/vibe/2026-09-22-3-replay-json-fidelity.md b/vibe/2026-09-22-3-replay-json-fidelity.md index 256834a6..021ae3ab 100644 --- a/vibe/2026-09-22-3-replay-json-fidelity.md +++ b/vibe/2026-09-22-3-replay-json-fidelity.md @@ -191,7 +191,7 @@ Add `install_deterministic_iteration` replacing `pairs`/`next` with sorted itera -### Step 6: Activate deterministic iteration in the section VM +### Step 6: Activate deterministic iteration in the section VM [completed] - Component: deterministic Lua iteration From 530a7a7d91f4057bb005831164fd38a06d7abb70 Mon Sep 17 00:00:00 2001 From: Vinnie Falco Date: Tue, 22 Sep 2026 15:41:05 -0700 Subject: [PATCH 13/19] =?UTF-8?q?=EF=BB=BFPin=20non-finite=20and=20canonic?= =?UTF-8?q?al-order=20JSON=20serialization?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replay comparison needs every payload to survive the log round trip exactly, but a non-finite floating point value has no JSON number form and is written as null, silently changing the value that was stored. Object keys need the same guarantee, because if insertion order ever reached the log two equal payloads could print differently. These tests pin both facts against the serializer so a later change that enables insertion-order preservation is caught. - `a_non_finite_f64_serializes_to_null` asserts that NaN, infinity, and negative infinity each serialize to the JSON literal null, and that a metrics record carrying a non-finite `mean_itl_ms` reaches the log as null instead of the value that went in. - `object_keys_serialize_in_canonical_order` asserts that a serialized object emits its keys sorted rather than in insertion order, since the value type is a sorted map and no crate enables insertion-order preservation. Plan: vibe/2026-09-22-3-replay-json-fidelity.md --- crates/promptforge-api-types/src/metrics.rs | 44 +++++++++++++++++++++ vibe/2026-09-22-3-replay-json-fidelity.md | 2 +- 2 files changed, 45 insertions(+), 1 deletion(-) diff --git a/crates/promptforge-api-types/src/metrics.rs b/crates/promptforge-api-types/src/metrics.rs index e37e855c..d68a7497 100644 --- a/crates/promptforge-api-types/src/metrics.rs +++ b/crates/promptforge-api-types/src/metrics.rs @@ -219,4 +219,48 @@ mod tests { "absent sections are omitted from the line" ); } + + /// A non-finite `f64` has no JSON number form, so `serde_json` writes it + /// as `null`; it would read back as a null or absent field, not the value + /// that went in. Replay comparison needs bit-for-bit equality, so the + /// producers reject non-finite values at the source instead of letting + /// the serializer erase them: a request temperature through + /// `Temperature::new`, and a timer's seconds through the task timeout + /// parse with the scheduler's defensive repeat. + #[test] + fn a_non_finite_f64_serializes_to_null() { + for non_finite in [f64::NAN, f64::INFINITY, f64::NEG_INFINITY] { + assert_eq!( + serde_json::to_string(&non_finite).expect("a non-finite float must serialize"), + "null", + "a non-finite float has no JSON number form" + ); + } + let metrics = VllmMetrics { + time_to_first_token_ms: None, + generation_time_ms: None, + queue_time_ms: None, + mean_itl_ms: Some(f64::NAN), + tokens_per_second: None, + }; + assert_eq!( + serde_json::to_string(&metrics).expect("metrics must serialize"), + r#"{"mean_itl_ms":null}"#, + "a non-finite timing reaches the log as null" + ); + } + + /// `Value::Object` is a `BTreeMap`, so serialization emits keys in sorted + /// order no matter the order they were inserted. No crate enables + /// `serde_json`'s `preserve_order`; were one to, insertion order would + /// leak into the log line and two equal payloads could print differently. + #[test] + fn object_keys_serialize_in_canonical_order() { + let value = json!({ "z": 1, "a": 2, "m": 3 }); + assert_eq!( + serde_json::to_string(&value).expect("a value must serialize"), + r#"{"a":2,"m":3,"z":1}"#, + "object keys must be sorted, not held in insertion order" + ); + } } diff --git a/vibe/2026-09-22-3-replay-json-fidelity.md b/vibe/2026-09-22-3-replay-json-fidelity.md index 021ae3ab..f6c8d18e 100644 --- a/vibe/2026-09-22-3-replay-json-fidelity.md +++ b/vibe/2026-09-22-3-replay-json-fidelity.md @@ -201,7 +201,7 @@ Call `install_deterministic_iteration` in `SectionVm::new` after `harden` in `cr -### Step 7: Record the non-finite and canonical-order regressions +### Step 7: Record the non-finite and canonical-order regressions [completed] - Component: hardening audit From d55876c1d12d1103e1fc8e78340967079e55e7da Mon Sep 17 00:00:00 2001 From: Vinnie Falco Date: Tue, 22 Sep 2026 15:44:14 -0700 Subject: [PATCH 14/19] =?UTF-8?q?=EF=BB=BFAudit=20ordered=20pair=20walks?= =?UTF-8?q?=20and=20sort=20schema=20required=20names?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The audit reviews every Rust-side walk over a Lua table that builds an ordered array rather than a value. Only the tool-parameter schema builder was leaving its output in the runtime's unspecified hash order, so it now sorts the declared names before emitting the schema. The other walks either already sort their output or only assign into a table or map, where the walk order cannot affect the result. That makes a stored tool definition's schema text identical in every process, so replay can compare it. - `required.sort()` imposes a bytewise order on the schema's required names, matching the shared `SortKey::Text` order the other ordered walks already use. - `add_local_params_schema` is the only ordered pair walk that needed the sort; `ordered_keys` and `collection_members` already sort their output, and `properties` sits in an order-stable map. - `argv.rs`, `coro.rs`, `error-value.rs`, `prose.rs`, and `sys.rs` gained only a recorded justification in each copy walk, because every assignment lands in a table or map whose contents are independent of walk order. Design: new pure-function @ crates/promptforge/lua/src/tools/decode.rs::add_local_params_schema deps: Table Repairs: the schema required array is emitted in a deterministic sorted order @ crates/promptforge/lua/src/tools/decode.rs::add_local_params_schema - the array followed the runtime's unspecified hash order, so the same parameters produced different schema text in two processes Plan: vibe/2026-09-22-3-replay-json-fidelity.md --- crates/promptforge/lua/src/argv.rs | 2 ++ crates/promptforge/lua/src/coro.rs | 2 ++ crates/promptforge/lua/src/error-value.rs | 2 ++ crates/promptforge/lua/src/prose.rs | 2 ++ crates/promptforge/lua/src/sys.rs | 2 ++ crates/promptforge/lua/src/tools/decode.rs | 13 +++++++- crates/promptforge/lua/src/tools/tests.rs | 36 ++++++++++++++++++---- vibe/2026-09-22-3-replay-json-fidelity.md | 2 +- 8 files changed, 53 insertions(+), 8 deletions(-) diff --git a/crates/promptforge/lua/src/argv.rs b/crates/promptforge/lua/src/argv.rs index e97c855c..58a627ea 100644 --- a/crates/promptforge/lua/src/argv.rs +++ b/crates/promptforge/lua/src/argv.rs @@ -82,6 +82,8 @@ pub(crate) fn install_frozen(lua: &Lua, argv: Option<&Json>) -> Result<()> { // Copy every other field the previous metatable installed, then shadow // the index pair with the argv guard. if let Some(old) = &old { + // `pairs` order is unspecified; each iteration only assigns one + // non-shadowed metatable field, so the copy's content is fixed. for pair in old.clone().pairs::() { let (key, value) = pair.map_err(Error::lua)?; let shadowed = diff --git a/crates/promptforge/lua/src/coro.rs b/crates/promptforge/lua/src/coro.rs index f54220f7..b37568f0 100644 --- a/crates/promptforge/lua/src/coro.rs +++ b/crates/promptforge/lua/src/coro.rs @@ -454,6 +454,8 @@ pub fn install_store_shims(lua: &Lua) -> Result<()> { .named_registry_value(STORE_REGISTRY) .map_err(Error::lua)?; let store: Table = lua.globals().raw_get("store").map_err(Error::lua)?; + // `pairs` order is unspecified; every iteration only assigns one named + // function into `store`, so the resulting table does not depend on it. for pair in shims.pairs::() { let (name, function) = pair.map_err(Error::lua)?; store.raw_set(name, function).map_err(Error::lua)?; diff --git a/crates/promptforge/lua/src/error-value.rs b/crates/promptforge/lua/src/error-value.rs index 0defb1b2..d371d76c 100644 --- a/crates/promptforge/lua/src/error-value.rs +++ b/crates/promptforge/lua/src/error-value.rs @@ -391,6 +391,8 @@ pub(crate) fn raised_from(lua: &Lua, value: &Value) -> mlua::Result message.to_str()?.to_owned(), _ => kind.tag().to_owned(), }; + // `pairs` order is unspecified, but `fields` is a `BTreeMap`, so the + // raised value's field order never depends on it. let mut fields = BTreeMap::new(); for pair in table.pairs::() { let (name, value) = pair?; diff --git a/crates/promptforge/lua/src/prose.rs b/crates/promptforge/lua/src/prose.rs index 51c1c876..bd1946b9 100644 --- a/crates/promptforge/lua/src/prose.rs +++ b/crates/promptforge/lua/src/prose.rs @@ -90,6 +90,8 @@ where // library's `_G` metatable keeps working), then shadow the index pair // with the prose guard. if let Some(old) = &old { + // `pairs` order is unspecified; each iteration only assigns one + // non-shadowed metatable field, so the copy's content is fixed. for pair in old.clone().pairs::() { let (key, value) = pair.map_err(Error::lua)?; let shadowed = diff --git a/crates/promptforge/lua/src/sys.rs b/crates/promptforge/lua/src/sys.rs index 36bfdafc..8494133f 100644 --- a/crates/promptforge/lua/src/sys.rs +++ b/crates/promptforge/lua/src/sys.rs @@ -107,6 +107,8 @@ fn materialize_guarded_value( _ => table, }; let plain = lua.create_table()?; + // `pairs` order is unspecified; each iteration only assigns one entry + // into the fresh table, so the copy's content is fixed. for pair in source.pairs::() { let (key, value) = pair?; plain.raw_set(key, materialize_guarded_value(lua, value, guarded_data)?)?; diff --git a/crates/promptforge/lua/src/tools/decode.rs b/crates/promptforge/lua/src/tools/decode.rs index 036c1542..62b960ce 100644 --- a/crates/promptforge/lua/src/tools/decode.rs +++ b/crates/promptforge/lua/src/tools/decode.rs @@ -109,6 +109,14 @@ pub(crate) fn collect_tools_add_entries(args: Variadic) -> mlua::Result mlua::Result { let mut properties = serde_json::Map::new(); let mut required = Vec::new(); @@ -134,8 +142,11 @@ pub(crate) fn add_local_params_schema(params: &mlua::Table) -> mlua::Result = - serde_json::from_value(schema["required"].clone()).expect("the required list is strings"); - required.sort(); - assert_eq!(required, vec!["limit", "query"]); + assert_eq!( + schema["required"], + json!(["limit", "query"]), + "required is emitted in sorted order, not Lua hash order" + ); assert_eq!(schema["type"], "object"); } +#[test] +fn add_local_params_schema_sorts_required_so_the_schema_text_is_deterministic() { + // `required` is the one Rust-side `table.pairs` walk whose output is an + // ordered array rather than a table or map, so it must impose an order + // itself: left as the walk produced it, two VMs emit different schema + // text for the same params, and a stored tool definition stops being + // replayable. + let lua = Lua::new(); + let params = lua + .load( + "{ zulu = 'string', alpha = 'string', mike = 'string', bravo = 'string', \ + yankee = 'string', charlie = 'string', oscar = 'string', delta = 'string' }", + ) + .eval::() + .expect("params table evaluates"); + let schema = add_local_params_schema(¶ms).expect("the schema builds"); + assert_eq!( + schema["required"], + json!([ + "alpha", "bravo", "charlie", "delta", "mike", "oscar", "yankee", "zulu" + ]), + "required must be sorted, not left in Lua hash order" + ); +} + #[test] fn add_local_params_schema_rejects_an_unsupported_type() { let lua = Lua::new(); diff --git a/vibe/2026-09-22-3-replay-json-fidelity.md b/vibe/2026-09-22-3-replay-json-fidelity.md index f6c8d18e..fe226244 100644 --- a/vibe/2026-09-22-3-replay-json-fidelity.md +++ b/vibe/2026-09-22-3-replay-json-fidelity.md @@ -211,7 +211,7 @@ Add a unit test in `crates/promptforge-api-types/src/metrics.rs` proving a non-f -### Step 8: Audit ordered pair walks +### Step 8: Audit ordered pair walks [completed] - Component: hardening audit From 52370760d3b1b32e195ec0a1de9f717195b94e01 Mon Sep 17 00:00:00 2001 From: Vinnie Falco Date: Tue, 22 Sep 2026 15:50:12 -0700 Subject: [PATCH 15/19] =?UTF-8?q?=EF=BB=BFAdd=20the=20JSON=20round-trip=20?= =?UTF-8?q?rule=20and=20sorted=20iteration=20order?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Record the repository rule that JSON reaching the run log or a replay comparison round-trips exactly, with canonical sorted object keys, finite numbers, and floats rounded to their meaningful precision at the source. Document that table iteration visits keys in a fixed sorted order, so every process observes the same sequence for the same table. - `AGENTS.md` states the round-trip rule: value identity across `to_value`, `to_string`, and `from_str`, canonical sorted object keys, finite numbers, floats rounded to meaningful precision at the source, and `preserve_order` never enabled. - `pairs` and `next` visit keys in a fixed sorted order - the array part first in index order, then booleans, numbers, and strings - recorded in both the language guide source and its `guide/src` chapter. Plan: vibe/2026-09-22-3-replay-json-fidelity.md --- AGENTS.md | 1 + guide/promptforge-language-guide.md | 2 +- guide/src/language/02-the-run.md | 2 +- vibe/2026-09-22-3-replay-json-fidelity.md | 2 +- 4 files changed, 4 insertions(+), 3 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 3f61d0e0..568fb009 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -44,6 +44,7 @@ Multi-crate Rust workspace for the PromptForge pipeline engine, the harness that - Long-running gateway work reports through `gateway-progress`, a private gateway family crate: a producer begins an activity with a text, replaces the text as work moves, and drops the guard when done. Consumers outside the family read only the `Progress` wire type from `gateway-api-types`. - Unsafe code stays in its explicitly owned boundary. Every unsafe block documents its safety invariants immediately before the block. - Comments explain a non-obvious constraint, ordering requirement, or workaround. Every platform or external-bug workaround cites its upstream issue URL in the explanatory comment. +- JSON that reaches the run log or a replay comparison round-trips exactly - `to_value`, `to_string`, `from_str` yield an identical value, object keys stay canonical (sorted), numbers must be finite, floats are rounded to their meaningful precision at the source, and serde_json `preserve_order` is never enabled. ## Verification diff --git a/guide/promptforge-language-guide.md b/guide/promptforge-language-guide.md index d0d3b3a7..b0523d75 100644 --- a/guide/promptforge-language-guide.md +++ b/guide/promptforge-language-guide.md @@ -89,7 +89,7 @@ Four calls are unavailable from the preamble: `call`, `jump`, `fanout`, and `lis After the preamble, the top-level sections run in file order. The first H2 section in the file is the entry point, and control falls through from each section to the next. -Each section runs in its own isolated, sandboxed Lua state. Only the `string`, `table`, and `math` standard libraries plus safe base functions are available. The state is created at section entry and torn down at exit, so one section's Lua cannot leak into the next. +Each section runs in its own isolated, sandboxed Lua state. Only the `string`, `table`, and `math` standard libraries plus safe base functions are available. The state is created at section entry and torn down at exit, so one section's Lua cannot leak into the next. The `pairs` and `next` iterators visit keys in a fixed sorted order: the array part first in index order, then booleans, numbers, and strings. A section that talks to the model needs a model. `models.use` selects a declared role by its label for one section, and the prompt-wide default covers sections that select nothing; a model-facing call with neither fails with a model-required error. Tools follow the same pattern: `tools.always` or `tools.add` advertise a bound tool to the model under its local alias. diff --git a/guide/src/language/02-the-run.md b/guide/src/language/02-the-run.md index 6710c825..8ead5766 100644 --- a/guide/src/language/02-the-run.md +++ b/guide/src/language/02-the-run.md @@ -20,7 +20,7 @@ Four calls are unavailable from the preamble: `call`, `jump`, `fanout`, and `lis After the preamble, the top-level sections run in file order. The first H2 section in the file is the entry point, and control falls through from each section to the next. -Each section runs in its own isolated, sandboxed Lua state. Only the `string`, `table`, and `math` standard libraries plus safe base functions are available. The state is created at section entry and torn down at exit, so one section's Lua cannot leak into the next. +Each section runs in its own isolated, sandboxed Lua state. Only the `string`, `table`, and `math` standard libraries plus safe base functions are available. The state is created at section entry and torn down at exit, so one section's Lua cannot leak into the next. The `pairs` and `next` iterators visit keys in a fixed sorted order: the array part first in index order, then booleans, numbers, and strings. A section that talks to the model needs a model. `models.use` selects a declared role by its label for one section, and the prompt-wide default covers sections that select nothing; a model-facing call with neither fails with a model-required error. Tools follow the same pattern: `tools.always` or `tools.add` advertise a bound tool to the model under its local alias. diff --git a/vibe/2026-09-22-3-replay-json-fidelity.md b/vibe/2026-09-22-3-replay-json-fidelity.md index fe226244..7cb8413f 100644 --- a/vibe/2026-09-22-3-replay-json-fidelity.md +++ b/vibe/2026-09-22-3-replay-json-fidelity.md @@ -221,7 +221,7 @@ Audit every Rust-side `table.pairs()` walk that produces an ordered `Vec` rather -### Step 9: Write the author-facing rules +### Step 9: Write the author-facing rules [completed] - Component: author-facing rules From d1b51a1f8d40246abd3d25306bb687e32641cd02 Mon Sep 17 00:00:00 2001 From: Vinnie Falco Date: Tue, 22 Sep 2026 16:03:36 -0700 Subject: [PATCH 16/19] =?UTF-8?q?=EF=BB=BFCorrect=20client-error=20log-lev?= =?UTF-8?q?el=20assertion=20to=20info?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A gateway test expected a caller-side client error to be logged at debug, but the code records those errors at info, so the assertion could not pass. The test now checks the info level and keeps asserting that no warning is emitted, leaving the warn stream reserved for server-side failures. - `client_error_logs_at_info_not_warn` asserts that a 4xx is logged at info and never at warn. Plan: vibe/2026-09-22-3-replay-json-fidelity.md --- crates/gateway/protocol/src/upstream.rs | 10 +++++----- vibe/2026-09-22-3-replay-json-fidelity.md | 2 +- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/crates/gateway/protocol/src/upstream.rs b/crates/gateway/protocol/src/upstream.rs index 6a375be9..a830f1fe 100644 --- a/crates/gateway/protocol/src/upstream.rs +++ b/crates/gateway/protocol/src/upstream.rs @@ -1181,10 +1181,10 @@ mod tests { } #[tokio::test] - async fn client_error_logs_at_debug_not_warn() { - // A 4xx is the caller's error, not the backend's: it logs at debug so + async fn client_error_logs_at_info_not_warn() { + // A 4xx is the caller's error, not the backend's: it logs at info so // the warn stream stays reserved for server-side failures. - let (logs, _guard) = capture_logs(tracing::Level::DEBUG); + let (logs, _guard) = capture_logs(tracing::Level::INFO); let body = r#"{"error":{"message":"unknown model","type":"invalid_request_error","code":"model_not_found"}}"#; let (base, handle) = serve_once("400 Bad Request", body); let upstream = OpenAiUpstream::new(&base, Secret::new(String::new())); @@ -1196,9 +1196,9 @@ mod tests { let logs = logs.contents(); assert!( logs.lines() - .any(|line| line.contains("DEBUG") + .any(|line| line.contains("INFO") && line.contains("upstream returned a client error")), - "a 4xx logs at debug: {logs}" + "a 4xx logs at info: {logs}" ); assert!( !logs diff --git a/vibe/2026-09-22-3-replay-json-fidelity.md b/vibe/2026-09-22-3-replay-json-fidelity.md index 7cb8413f..256c69ac 100644 --- a/vibe/2026-09-22-3-replay-json-fidelity.md +++ b/vibe/2026-09-22-3-replay-json-fidelity.md @@ -231,7 +231,7 @@ Add the round-trip rule to the Engineering section of `AGENTS.md` and the sorted -### Step 10: Run the exit checks +### Step 10: Run the exit checks [completed] - Component: verification gate From 01535c431d4c6cae0ce51870395e64ea1d22dd42 Mon Sep 17 00:00:00 2001 From: Vinnie Falco Date: Tue, 22 Sep 2026 16:10:56 -0700 Subject: [PATCH 17/19] Close plan: replay json fidelity Plan: vibe/2026-09-22-3-replay-json-fidelity.md --- vibe/ACTIVE | 1 - 1 file changed, 1 deletion(-) delete mode 100644 vibe/ACTIVE diff --git a/vibe/ACTIVE b/vibe/ACTIVE deleted file mode 100644 index 06ec519f..00000000 --- a/vibe/ACTIVE +++ /dev/null @@ -1 +0,0 @@ -vibe/2026-09-22-3-replay-json-fidelity.md From 1ef07395e9b2478f6434d1f461bec52ad85844f8 Mon Sep 17 00:00:00 2001 From: Vinnie Falco Date: Tue, 22 Sep 2026 16:42:01 -0700 Subject: [PATCH 18/19] =?UTF-8?q?=EF=BB=BFFix=20deterministic=20iteration?= =?UTF-8?q?=20resumption=20on=20mid-traversal=20clears?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The deterministic pairs/next walker could revisit keys or end the walk early when a key was cleared during traversal: its ordered key list and its resumption comparator disagreed, and resumption required the previous key to be sortable. Share one walk order between the list and the resumption, and let pairs advance a captured snapshot, so a cleared key of any type is skipped and the walk stays strictly forward. - iteration.rs: one array < scalar-hash < non-scalar walk order; next is strictly forward for present keys with a best-effort fallback. - pairs returns (iterator, table, nil) and advances the snapshot by index. - iteration-tests.rs: the boolean-clear and non-scalar-clear repros plus next-path coverage; all fail against the pre-fix walker. Plan: none --- crates/promptforge/lua/src/iteration-tests.rs | 100 +++++++++ crates/promptforge/lua/src/iteration.rs | 189 +++++++++++++----- 2 files changed, 237 insertions(+), 52 deletions(-) diff --git a/crates/promptforge/lua/src/iteration-tests.rs b/crates/promptforge/lua/src/iteration-tests.rs index 9c00ab42..1b1e9a99 100644 --- a/crates/promptforge/lua/src/iteration-tests.rs +++ b/crates/promptforge/lua/src/iteration-tests.rs @@ -102,6 +102,58 @@ fn next_returns_the_first_pair_of_a_non_empty_table() { assert_eq!(sequence(&lua, source), vec![json!(true), json!(true)]); } +#[test] +fn next_resumes_after_a_cleared_boolean_key_without_replaying_the_array_part() { + // The stateless `next` rebuilds the walk from the live table. A boolean + // hash key ranks below numbers in `SortKey`, so a resume that compared raw + // sort positions would place the array segment after the cleared boolean + // and replay it. Clearing each boolean key mid-walk must still visit every + // key exactly once and stop at the end. + let lua = vm(); + let source = "local t = {10, 20, [false] = 'f', [true] = 't'} \ + local out = {} \ + for k in next, t do \ + if type(k) == 'boolean' then \ + out[#out+1] = tostring(k) \ + t[k] = nil \ + else \ + out[#out+1] = k \ + end \ + end \ + return out"; + assert_eq!( + sequence(&lua, source), + vec![json!(1), json!(2), json!("false"), json!("true")] + ); +} + +#[test] +fn next_resumes_after_a_cleared_non_scalar_key() { + // A cleared non-scalar key has no computed sort position, so `next` falls + // back to the trailing segment without ending the walk. Clearing one + // table-valued key must still yield the other, then terminate, however the + // raw `pairs` order placed the two. + let lua = vm(); + let source = "local t = {a = 1} \ + local first, second = {}, {} \ + t[first] = 'f' \ + t[second] = 's' \ + local out = {} \ + for k in next, t do \ + if type(k) == 'table' then \ + out[#out+1] = 'key' \ + t[k] = nil \ + else \ + out[#out+1] = k \ + end \ + end \ + return out"; + assert_eq!( + sequence(&lua, source), + vec![json!("a"), json!("key"), json!("key")] + ); +} + #[test] fn pairs_skips_a_key_cleared_before_it_is_visited() { let lua = vm(); @@ -133,3 +185,51 @@ fn pairs_skips_the_key_cleared_by_the_current_step() { vec![json!("a"), json!("b"), json!("c")] ); } + +#[test] +fn pairs_does_not_revisit_the_array_part_when_a_boolean_key_is_cleared() { + // D-003. A boolean hash key ranks below numbers in `SortKey`, so a + // resumption that compares raw sort positions places the array part after + // the boolean key and replays it. The array part, the boolean key, and the + // clear must all coexist for the defect to surface; each key must still be + // yielded exactly once. + let lua = vm(); + let source = "local t = {10, 20, [false] = 'f'} \ + local out = {} \ + for k in pairs(t) do \ + out[#out+1] = k \ + if k == false then t[k] = nil end \ + end \ + return out"; + assert_eq!( + sequence(&lua, source), + vec![json!(1), json!(2), json!(false)] + ); +} + +#[test] +fn pairs_visits_remaining_non_scalar_keys_after_one_is_cleared() { + // D-002. Two non-scalar keys sort after every scalar key; clearing the + // current table-valued key mid-loop must not end the walk, so the other + // table key is still visited. Both clear, so the count pins the fix without + // depending on the raw `pairs` order between the two table keys. + let lua = vm(); + let source = "local t = {a = 1} \ + local first, second = {}, {} \ + t[first] = 'f' \ + t[second] = 's' \ + local out = {} \ + for k in pairs(t) do \ + if type(k) == 'table' then \ + out[#out+1] = 'key' \ + t[k] = nil \ + else \ + out[#out+1] = k \ + end \ + end \ + return out"; + assert_eq!( + sequence(&lua, source), + vec![json!("a"), json!("key"), json!("key")] + ); +} diff --git a/crates/promptforge/lua/src/iteration.rs b/crates/promptforge/lua/src/iteration.rs index e5781e41..0bdb032f 100644 --- a/crates/promptforge/lua/src/iteration.rs +++ b/crates/promptforge/lua/src/iteration.rs @@ -7,20 +7,25 @@ //! [`SortKey`](crate::collection::SortKey) order (booleans, then numbers, then //! strings). A `__pairs` metamethod still wins, exactly as in stock Lua. //! -//! `next` is stateless: each call rebuilds the ordered key sequence from the -//! live table, so a key whose value became `nil` mid-traversal is skipped -//! rather than visited with a nil value, and traversal always advances to a -//! key strictly after the previous one. Rebuilding costs `O(n log n)` per -//! step; section tables are small and the determinism is the point. The -//! ordering covers string, number, and boolean keys, the only keys a payload -//! the run log stores can carry. A non-scalar key (table, function, userdata, -//! or thread) is still visited rather than dropped, but it sits after every -//! scalar key in the order Lua's own `next` produced it: such a key has no -//! cross-process position, and it cannot back a logged value. +//! `pairs` captures that order once and advances it by position, so a key +//! whose value becomes `nil` mid-traversal is skipped rather than visited with +//! a nil value, whatever its type: a cleared key is skipped without a lookup +//! that would need it to be sortable. `next` stays stateless and rebuilds the +//! ordered key sequence from the live table on each call; for a key still +//! present it advances strictly, and for a cleared scalar key it resumes at +//! the first key after the cleared key's sort position. A cleared non-scalar +//! key has no cross-process position, so `next` falls back to the trailing +//! segment such keys occupy, the best a stateless resume can do. Rebuilding +//! costs `O(n log n)` per step; section tables are small and the determinism +//! is the point. The ordering covers string, number, and boolean keys, the +//! only keys a payload the run log stores can carry. A non-scalar key (table, +//! function, userdata, or thread) is still visited rather than dropped, but it +//! sits after every scalar key in the order Lua's own `next` produced it: such +//! a key has no cross-process position, and it cannot back a logged value. use std::cmp::Ordering; -use mlua::{Function, Lua, MultiValue, Table, Value}; +use mlua::{Lua, MultiValue, Table, Value}; use crate::collection::{SortKey, sort_key}; use crate::error::{Error, Result}; @@ -36,12 +41,52 @@ pub(crate) fn install_deterministic_iteration(lua: &Lua) -> Result<()> { .create_function(deterministic_next) .map_err(Error::lua)?; globals.raw_set("next", &next_fn).map_err(Error::lua)?; - let pairs_fn = lua - .create_function(move |_, value: Value| pairs(value, &next_fn)) - .map_err(Error::lua)?; + let pairs_fn = lua.create_function(pairs).map_err(Error::lua)?; globals.raw_set("pairs", pairs_fn).map_err(Error::lua) } +/// One table key and its position in the deterministic walk order. +struct WalkKey { + key: Value, + order: WalkOrder, +} + +/// Where a key sits in the walk order: the array segment, the scalar hash +/// segment (by [`SortKey`]), or the trailing non-scalar segment. The segments +/// compare before the inner position, so an array key always precedes a +/// boolean hash key even though [`SortKey`] ranks a boolean below a number; +/// the ordered list and the resumption comparator thus agree. A non-scalar key +/// has no cross-position, so all of them compare equal and keep their raw +/// `pairs` order. +enum WalkOrder { + Array(i64), + Scalar(SortKey), + Unordered, +} + +impl WalkOrder { + /// The segment rank: the array part, then scalar hash keys, then + /// non-scalar keys. Segments compare before any within-segment position. + fn segment(&self) -> u8 { + match self { + WalkOrder::Array(_) => 0, + WalkOrder::Scalar(_) => 1, + WalkOrder::Unordered => 2, + } + } + + /// Orders two walk positions by segment, then within the segment. All + /// non-scalar positions compare equal, so a stable sort keeps their raw + /// `pairs` order. + fn compare(&self, other: &Self) -> Ordering { + match (self, other) { + (WalkOrder::Array(left), WalkOrder::Array(right)) => left.cmp(right), + (WalkOrder::Scalar(left), WalkOrder::Scalar(right)) => left.compare(right), + _ => self.segment().cmp(&other.segment()), + } + } +} + /// The `next(table, key)` replacement: the key strictly after `key` in the /// sorted order, with its value, or `(nil, nil)` at the end. fn deterministic_next( @@ -49,17 +94,17 @@ fn deterministic_next( (table, previous): (Table, Value), ) -> mlua::Result<(Value, Value)> { let keys = ordered_keys(&table).map_err(mlua::Error::external)?; - let Some(index) = next_index(&keys, &previous) else { + let Some(index) = next_index(&keys, &previous, table.raw_len()) else { return Ok((Value::Nil, Value::Nil)); }; - let key = keys[index].clone(); + let key = keys[index].key.clone(); let value: Value = table.raw_get(&key)?; Ok((key, value)) } /// The `pairs(table)` replacement: a `__pairs` metamethod's results when one -/// is present, otherwise `(next, table, nil)` over the installed `next`. -fn pairs(value: Value, next_fn: &Function) -> mlua::Result { +/// is present, otherwise a stateful iterator over the table's walk order. +fn pairs(lua: &Lua, value: Value) -> mlua::Result { let table = match &value { Value::Table(table) => table.clone(), other => { @@ -81,19 +126,41 @@ fn pairs(value: Value, next_fn: &Function) -> mlua::Result { } } } - let mut iterator = MultiValue::new(); - iterator.push_back(Value::Function(next_fn.clone())); - iterator.push_back(value); - iterator.push_back(Value::Nil); - Ok(iterator) + // Capture the walk order once, then advance it by position. Advancing a + // snapshot rather than re-deriving order from the live table makes the + // walk independent of a key's sortability: a key cleared mid-loop reads as + // nil and is skipped, and no key-based lookup can end the walk early. + let keys = ordered_keys(&table)?; + // The VM is single-threaded, so the cursor is a plain `usize` mutated in + // place rather than an atomic. + let state = table.clone(); + let mut position = 0usize; + let iterator = lua.create_function_mut(move |_, ()| { + while let Some(entry) = keys.get(position) { + position += 1; + let value: Value = table.raw_get(&entry.key)?; + if !matches!(value, Value::Nil) { + return Ok((entry.key.clone(), value)); + } + } + Ok((Value::Nil, Value::Nil)) + })?; + // Stock `pairs` yields `(next, table, nil)`; the iterator ignores the + // state, but keeping the shape means a caller reading the second result + // sees the table, exactly as with the pre-replacement `pairs`. + let mut iterator_value = MultiValue::new(); + iterator_value.push_back(Value::Function(iterator)); + iterator_value.push_back(Value::Table(state)); + iterator_value.push_back(Value::Nil); + Ok(iterator_value) } /// The table's live keys in deterministic walk order: the array part /// (`1..=#table`) in index order, then the hash part's scalar keys by /// [`SortKey`], then any non-scalar keys. -fn ordered_keys(table: &Table) -> mlua::Result> { +fn ordered_keys(table: &Table) -> mlua::Result> { let border = table.raw_len(); - let mut keys: Vec = Vec::new(); + let mut keys: Vec = Vec::new(); for index in 1..=border { let value: Value = table.raw_get(index)?; if matches!(value, Value::Nil) { @@ -102,10 +169,13 @@ fn ordered_keys(table: &Table) -> mlua::Result> { let index = i64::try_from(index).map_err(|_| { mlua::Error::runtime("table index exceeds the integer range".to_owned()) })?; - keys.push(Value::Integer(index)); + keys.push(WalkKey { + key: Value::Integer(index), + order: WalkOrder::Array(index), + }); } - let mut hashed: Vec<(SortKey, Value)> = Vec::new(); - let mut unordered: Vec = Vec::new(); + let mut hashed: Vec = Vec::new(); + let mut unordered: Vec = Vec::new(); for pair in table.pairs::() { let (key, value) = pair?; if matches!(value, Value::Nil) { @@ -117,43 +187,58 @@ fn ordered_keys(table: &Table) -> mlua::Result> { continue; } match sort_key(&key) { - Ok((position, _)) => hashed.push((position, key)), - Err(_) => unordered.push(key), + Ok((position, _)) => hashed.push(WalkKey { + key, + order: WalkOrder::Scalar(position), + }), + Err(_) => unordered.push(WalkKey { + key, + order: WalkOrder::Unordered, + }), } } - hashed.sort_by(|left, right| left.0.compare(&right.0)); - keys.extend(hashed.into_iter().map(|(_, key)| key)); + hashed.sort_by(|left, right| left.order.compare(&right.order)); + keys.extend(hashed); keys.extend(unordered); Ok(keys) } /// The index in `keys` to return for a `next` call with `previous` as the -/// last key. An exact match advances one slot; a key whose value was cleared +/// last key. An exact match advances one slot. A key whose value was cleared /// (`previous` absent) resumes at the first key that sorts strictly after it, -/// so the cleared key is skipped rather than revisited. -fn next_index(keys: &[Value], previous: &Value) -> Option { +/// so the cleared key is skipped rather than revisited; a cleared non-scalar +/// key has no computed position, so the resume falls back to the trailing +/// segment, the best a stateless resume can offer. +fn next_index(keys: &[WalkKey], previous: &Value, border: usize) -> Option { if matches!(previous, Value::Nil) { return (!keys.is_empty()).then_some(0); } - if let Some(index) = keys.iter().position(|key| key == previous) { - return (index + 1 < keys.len()).then_some(index + 1); + if let Some(position) = keys.iter().position(|entry| &entry.key == previous) { + return (position + 1 < keys.len()).then_some(position + 1); } - let Ok((position, _)) = sort_key(previous) else { - return None; - }; - let mut unordered = None; - for (index, key) in keys.iter().enumerate() { - match sort_key(key) { - Ok((candidate, _)) if candidate.compare(&position) == Ordering::Greater => { - return Some(index); - } - Err(_) => { - unordered.get_or_insert(index); - } - Ok(_) => {} - } + let order = previous_order(previous, border); + if matches!(order, WalkOrder::Unordered) { + return keys + .iter() + .position(|entry| matches!(entry.order, WalkOrder::Unordered)); + } + keys.iter() + .position(|entry| entry.order.compare(&order) == Ordering::Greater) +} + +/// The walk position a lone key would occupy, ignoring whether it is still in +/// the table: an integer inside the array border is an array key, a scalar +/// hash key keeps its [`SortKey`], and anything else is non-scalar. +fn previous_order(previous: &Value, border: usize) -> WalkOrder { + if let Value::Integer(index) = previous + && usize::try_from(*index).is_ok_and(|index| (1..=border).contains(&index)) + { + return WalkOrder::Array(*index); + } + match sort_key(previous) { + Ok((position, _)) => WalkOrder::Scalar(position), + Err(_) => WalkOrder::Unordered, } - unordered } #[cfg(test)] From 717102979048b555d69f02e3223477370a6e4f76 Mon Sep 17 00:00:00 2001 From: Vinnie Falco Date: Tue, 22 Sep 2026 16:48:00 -0700 Subject: [PATCH 19/19] =?UTF-8?q?=EF=BB=BFCorrect=20stale=20audit=20and=20?= =?UTF-8?q?rounding-rule=20wording?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two documentation claims no longer matched the code: the replay-fidelity plan's audit line called the decode.rs walk canonical even though its required array was unsorted and is now sorted, and the Engineering rule implied every logged float is rounded at the source when only derived client timings are. State each accurately. - vibe/2026-09-22-3-replay-json-fidelity.md: the decode.rs audit line. - AGENTS.md: separate the exact-parsing guarantee from source rounding. Plan: none --- AGENTS.md | 2 +- vibe/.gitignore | 2 ++ vibe/2026-09-22-3-replay-json-fidelity.md | 2 +- 3 files changed, 4 insertions(+), 2 deletions(-) create mode 100644 vibe/.gitignore diff --git a/AGENTS.md b/AGENTS.md index 568fb009..e4407629 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -44,7 +44,7 @@ Multi-crate Rust workspace for the PromptForge pipeline engine, the harness that - Long-running gateway work reports through `gateway-progress`, a private gateway family crate: a producer begins an activity with a text, replaces the text as work moves, and drops the guard when done. Consumers outside the family read only the `Progress` wire type from `gateway-api-types`. - Unsafe code stays in its explicitly owned boundary. Every unsafe block documents its safety invariants immediately before the block. - Comments explain a non-obvious constraint, ordering requirement, or workaround. Every platform or external-bug workaround cites its upstream issue URL in the explanatory comment. -- JSON that reaches the run log or a replay comparison round-trips exactly - `to_value`, `to_string`, `from_str` yield an identical value, object keys stay canonical (sorted), numbers must be finite, floats are rounded to their meaningful precision at the source, and serde_json `preserve_order` is never enabled. +- JSON that reaches the run log or a replay comparison round-trips exactly - `to_value`, `to_string`, `from_str` yield an identical value, object keys stay canonical (sorted), numbers must be finite, and serde_json `preserve_order` is never enabled. Exact parsing (`float_roundtrip`) carries that guarantee; a value derived and then logged is additionally rounded to its meaningful precision at the source. ## Verification diff --git a/vibe/.gitignore b/vibe/.gitignore new file mode 100644 index 00000000..21d25e6f --- /dev/null +++ b/vibe/.gitignore @@ -0,0 +1,2 @@ +/scratch + diff --git a/vibe/2026-09-22-3-replay-json-fidelity.md b/vibe/2026-09-22-3-replay-json-fidelity.md index 256c69ac..f0546358 100644 --- a/vibe/2026-09-22-3-replay-json-fidelity.md +++ b/vibe/2026-09-22-3-replay-json-fidelity.md @@ -83,7 +83,7 @@ Tests pin the log round-trip and the iteration order, and the change is gated by - Unit: in `crates/promptforge/model-client/src/client/read-tests.rs`, a `ClientTiming` built from a `Duration` with nanosecond noise serializes to a short decimal and `from_str(to_string(v))` equals `v` for all three fields, adjusting the existing `EPSILON` assertions (lines 129-131). In the Lua crate, `iteration-tests.rs` asserts string keys iterate in byte order regardless of insertion order, the array part precedes the hash part, mixed key types order bool < number < string, `__pairs` is honored, `next(t) == nil` on an empty table, a cleared field is skipped mid-traversal, and two fresh VMs produce the identical order for the same table. - Integration and end-to-end: a new `crates/harness/log/tests/it/fidelity.rs` appends records whose payloads hold awkward floats (`0.1 + 0.2`, `3.9078000000000004`, `1e-7`, `1e21`, `f64::MAX`, negative zero), nested objects built with keys in non-sorted insertion order, arrays of mixed numbers, and a real `Event::AssistantReply` with `CallMetrics`; reading back asserts `stored.payload == original` and `to_string(&stored.payload) == to_string(&original)`. Run `cargo nextest run -p harness-models a_prepared_run_drives_end_to_end` twenty times in a loop; all must pass. -- Regression, security, and performance: confirm no crate enables `preserve_order` (so key order stays canonical) and none enables `float_roundtrip` today; add a unit test in `crates/promptforge-api-types/src/metrics.rs` proving that serializing a non-finite `f64` yields `null`, documenting why producers reject non-finite values at the source; audit every Rust-side `table.pairs()` walk that produces an ordered `Vec` rather than a `Value` (known sites: `crates/promptforge/lua/src/collection.rs`, already sorted, and `crates/promptforge/lua/src/tools/decode.rs:115`, which sinks into an object and is canonical) and sort or record each. +- Regression, security, and performance: confirm no crate enables `preserve_order` (so key order stays canonical) and none enables `float_roundtrip` today; add a unit test in `crates/promptforge-api-types/src/metrics.rs` proving that serializing a non-finite `f64` yields `null`, documenting why producers reject non-finite values at the source; audit every Rust-side `table.pairs()` walk that produces an ordered `Vec` rather than a `Value` (known sites: `crates/promptforge/lua/src/collection.rs`, already sorted, and `crates/promptforge/lua/src/tools/decode.rs`, whose ordered `required` array was unsorted and is now sorted) and sort or record each. - Exit criteria: `cargo fmt --all --check`; workspace clippy with `-D warnings`; `cargo hakari verify`; `cargo test -p build-xtask` (new files stay within the repository's file-size convention); rustdoc with `RUSTDOCFLAGS="-D warnings"`; the full nextest suite plus doctests; the twenty-run flake loop; `mdbook build guide`.