diff --git a/AGENTS.md b/AGENTS.md index 3f61d0e01..e44076299 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, 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/Cargo.lock b/Cargo.lock index 8a8d6fd49..624233689 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", @@ -2668,6 +2669,7 @@ dependencies = [ name = "harness-sessions" version = "0.3.0" dependencies = [ + "axum", "harness-capabilities", "harness-log", "harness-models", @@ -8739,7 +8741,6 @@ dependencies = [ "tar", "tauri-utils", "time", - "time-macros", "tinystr", "tokio", "tokio-util", diff --git a/Cargo.toml b/Cargo.toml index f2b18f9a1..8a27de511 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/gateway/protocol/src/upstream.rs b/crates/gateway/protocol/src/upstream.rs index 019a059a0..a830f1fea 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::info!( + 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_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::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())); + 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("INFO") + && line.contains("upstream returned a client error")), + "a 4xx logs at info: {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/crates/harness/log/Cargo.toml b/crates/harness/log/Cargo.toml index fb2aeaec4..da01442fa 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 c519d26ac..22ad75cc1 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 000000000..97de14f74 --- /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 05762c1d4..d73ae7596 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/crates/harness/sessions/Cargo.toml b/crates/harness/sessions/Cargo.toml index da943f45a..e05885ca7 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 000000000..ee222044a --- /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 e5266e2a5..03d65f964 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 000000000..a8b9209f2 --- /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 ec24f1eea..909cc9df7 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 19d60bcbf..dc94066a3 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 405b5cd42..1d1540dfc 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.rs b/crates/promptforge-api-runtime/src/execute/tests.rs index c0aab8c90..115180af3 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/chat_arm.rs b/crates/promptforge-api-runtime/src/execute/tests/chat_arm.rs index c9d14f863..4f4f302f1 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 31bbba6f5..550c2a233 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/tests/models_loop.rs b/crates/promptforge-api-runtime/src/execute/tests/models_loop.rs index d116c9b34..c1e437e9c 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-api-runtime/src/execute/tools.rs b/crates/promptforge-api-runtime/src/execute/tools.rs index 9db99e281..c7c13d3ba 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 fe7f70e86..41f82aa6f 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 67f8102a9..5b8192cec 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 49fe0054d..279b8efff 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 6195f6baa..d522667ac 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 b23fb730b..50bf4bcab 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 e80041d9e..f2b6b0b6d 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-api-types/src/metrics.rs b/crates/promptforge-api-types/src/metrics.rs index e37e855c5..d68a74975 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/crates/promptforge/lua/src/argv.rs b/crates/promptforge/lua/src/argv.rs index e97c855c6..58a627ea1 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/collection-order-tests.rs b/crates/promptforge/lua/src/collection-order-tests.rs new file mode 100644 index 000000000..ecbae7cab --- /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 000000000..612ccaca1 --- /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 1263af4d9..bccb9c152 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/crates/promptforge/lua/src/coro.rs b/crates/promptforge/lua/src/coro.rs index f54220f74..b37568f06 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 0defb1b21..d371d76cf 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/iteration-tests.rs b/crates/promptforge/lua/src/iteration-tests.rs new file mode 100644 index 000000000..1b1e9a99e --- /dev/null +++ b/crates/promptforge/lua/src/iteration-tests.rs @@ -0,0 +1,235 @@ +//! 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 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(); + 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")] + ); +} + +#[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 new file mode 100644 index 000000000..0bdb032fc --- /dev/null +++ b/crates/promptforge/lua/src/iteration.rs @@ -0,0 +1,246 @@ +//! 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. +//! +//! `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::{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(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( + _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, table.raw_len()) else { + return Ok((Value::Nil, Value::Nil)); + }; + 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 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 => { + 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() + ))); + } + } + } + // 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> { + 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(WalkKey { + key: Value::Integer(index), + order: WalkOrder::Array(index), + }); + } + 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) { + 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(WalkKey { + key, + order: WalkOrder::Scalar(position), + }), + Err(_) => unordered.push(WalkKey { + key, + order: WalkOrder::Unordered, + }), + } + } + 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 +/// (`previous` absent) resumes at the first key that sorts strictly after it, +/// 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(position) = keys.iter().position(|entry| &entry.key == previous) { + return (position + 1 < keys.len()).then_some(position + 1); + } + 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, + } +} + +#[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 b3e226e90..744f9e603 100644 --- a/crates/promptforge/lua/src/lib.rs +++ b/crates/promptforge/lua/src/lib.rs @@ -95,7 +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; +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/projection-tests.rs b/crates/promptforge/lua/src/projection-tests.rs index e812fb1bd..93c511888 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 78f8df768..269a9a97c 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/prose.rs b/crates/promptforge/lua/src/prose.rs index 51c1c8767..bd1946b9c 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/protocol/request.rs b/crates/promptforge/lua/src/protocol/request.rs index 1bd70ed2a..4b548d271 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/lua/src/protocol/tests/answer.rs b/crates/promptforge/lua/src/protocol/tests/answer.rs index c6dd839b0..9869fe6b0 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/promptforge/lua/src/sys.rs b/crates/promptforge/lua/src/sys.rs index 36bfdafc8..8494133f8 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/tests.rs b/crates/promptforge/lua/src/tests.rs index 4beaceb26..18e44531a 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/tools/decode.rs b/crates/promptforge/lua/src/tools/decode.rs index 036c15424..62b960ce3 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/crates/promptforge/lua/src/vm.rs b/crates/promptforge/lua/src/vm.rs index 756d693d6..7099dd664 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/crates/promptforge/model-client/src/client/read-tests.rs b/crates/promptforge/model-client/src/client/read-tests.rs index 1be33926e..3ed1c42ae 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 4e414f9c4..92e37df1f 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/crates/promptforge/model-client/src/client/wire.rs b/crates/promptforge/model-client/src/client/wire.rs index 0361695b3..993d5ef9a 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/crates/workshop/protocol/tests/it/fixture.rs b/crates/workshop/protocol/tests/it/fixture.rs index 62212b0a2..a63f7368c 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 dbd2dc6a1..70b09d3d4 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, } } diff --git a/crates/workspace-hack/Cargo.toml b/crates/workspace-hack/Cargo.toml index 7d0f80268..7e5af2a64 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/guide/promptforge-agent-guide.md b/guide/promptforge-agent-guide.md index 52450f933..66de25d91 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/promptforge-language-guide.md b/guide/promptforge-language-guide.md index d0d3b3a78..b0523d755 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/agent/05-the-event-log.md b/guide/src/agent/05-the-event-log.md index 434edaa88..393237d64 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/guide/src/language/02-the-run.md b/guide/src/language/02-the-run.md index 6710c8250..8ead57664 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/.gitignore b/vibe/.gitignore new file mode 100644 index 000000000..21d25e6f2 --- /dev/null +++ b/vibe/.gitignore @@ -0,0 +1,2 @@ +/scratch + 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 000000000..25f11b333 --- /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 [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. +- 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/2026-09-22-2-emit-infer-reply.md b/vibe/2026-09-22-2-emit-infer-reply.md new file mode 100644 index 000000000..b0d113223 --- /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/2026-09-22-3-replay-json-fidelity.md b/vibe/2026-09-22-3-replay-json-fidelity.md new file mode 100644 index 000000000..f05463588 --- /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`, 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`. + + + + +## 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 [completed] + +- 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 [completed] + +- 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 [completed] + +- 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 [completed] + +- 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 [completed] + +- 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 [completed] + +- 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 [completed] + +- 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 [completed] + +- 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 [completed] + +- 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/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 diff --git a/vibe/papergate-harness-migration.md b/vibe/papergate-harness-migration.md index be7c8630b..80119b350 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.