diff --git a/CHANGELOG.md b/CHANGELOG.md index 17d7edf..a9f32e7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,17 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added +- **`RunInput::with_run_id`** — a host names a run with a durable, + server-generated id, seeded into the run state as `run.id`. The engine's own + run id is process-local and minted fresh on every call (a resume re-executes, + so it changes between a run and its own resume), which makes it unusable for + anything that must name *this* run across a pause. The `approval` node's + `request_id` defaults to `":"` and so is now unique per run + **and** stable across resume — the property that makes one human review + rather than a fresh card per resume. Seeded outside `run.trigger` on purpose: + the trigger is caller-supplied and, on a webhook, attacker-influenced, and the + review-de-duplication key must not be. + - **`approval` node kind + the `ApprovalProvider` capability** — a human-in-the-loop review step that carries what is being reviewed (a URL, a draft, any payload) and routes on the answer: `approved` / `rejected` ports, diff --git a/examples/hitl_review.rs b/examples/hitl_review.rs index 2a931c3..e73abe2 100644 --- a/examples/hitl_review.rs +++ b/examples/hitl_review.rs @@ -24,7 +24,7 @@ async fn main() { ApprovalDecision, ApprovalOutcome, ApprovalProvider, ApprovalRequest, Capabilities, }; use tinyflows::compiler::compile; - use tinyflows::engine::{resume, run}; + use tinyflows::engine::{RunInput, resume, run}; use tinyflows::model::{Edge, Node, NodeKind, WorkflowGraph}; /// A host's review desk: one row per `request_id`, holding the verdict once @@ -108,7 +108,6 @@ async fn main() { // A real host would key this on the run id (e.g. // `"=run.id"`) rather than a literal, so two runs of this // graph never collide on the same review. - "request_id": "hitl-review-example", "title": "Publish this post?", "prompt": "Approving publishes it to the public feed.", "subject_kind": "url", @@ -141,11 +140,18 @@ async fn main() { approvals: Some(desk.clone()), ..mock_capabilities() }; + // The host names the run. This is what gives the review a stable identity + // (`request_id` defaults to ":"), so the resume below + // resolves the card already in front of a person instead of opening a + // second one. It is seeded outside the trigger payload on purpose: a + // caller-supplied value here would hand an attacker the de-duplication key. + let run_id = "run-7f3a"; let trigger = json!({ "url": "https://example.com/drafts/42" }); + let input = || RunInput::new(trigger.clone()).with_run_id(run_id); // 1) Nobody has answered, so the run suspends at the review. Nothing is // burned while the card sits in someone's queue. - let paused = run(&compiled, trigger.clone(), &caps).await.expect("run"); + let paused = run(&compiled, input(), &caps).await.expect("run"); println!("--- before the human answers ---"); println!("pending_approvals: {:?}", paused.pending_approvals); println!("desk queue: {:?}", desk.queue()); @@ -165,7 +171,7 @@ async fn main() { // 3) Resuming re-asks the desk, which now has the verdict. Note the review // id is unchanged, so the reviewer is never asked a second time. - let done = resume(&compiled, trigger, vec![], &caps) + let done = resume(&compiled, input(), vec![], &caps) .await .expect("resume"); println!("--- after the human answers ---"); diff --git a/src/catalog/contracts/group_03.rs b/src/catalog/contracts/group_03.rs index 8771894..51b4fac 100644 --- a/src/catalog/contracts/group_03.rs +++ b/src/catalog/contracts/group_03.rs @@ -166,7 +166,9 @@ pub(super) fn contract_approval() -> NodeKindContract { whichever of `run.id` / `run.run_id` the host seeds — never read from the \ caller-supplied trigger payload). \ Must be stable across resumes: it is the key the host de-duplicates reviews on. \ - Required when no run-scoped id is available — falling back to the bare node id \ + A host names the run with `RunInput::with_run_id`, which lands in `run.id`; \ + with that seeded this field is optional. Required only when no run-scoped id \ + is available — falling back to the bare node id \ would let a later run of the same graph reuse an earlier run's decision, so the \ node refuses to guess and fails instead. SECURITY: whichever run id feeds this \ must be server-generated, never a caller-supplied trigger field forwarded \ diff --git a/src/engine.rs b/src/engine.rs index 939815e..9b90935 100644 --- a/src/engine.rs +++ b/src/engine.rs @@ -146,6 +146,31 @@ pub struct RunInput { /// array) has nowhere to put them, so smuggling approvals through the /// payload cannot work in general. pub approvals: Vec, + /// A durable, **host-generated** identity for this run, seeded into the run + /// state as `run.id`. + /// + /// The engine keeps its own process-local run id for observability, but + /// that one is minted fresh inside every `run` call — and a resume + /// re-executes the workflow, so it changes between a run and its own + /// resume. Anything that must name *this* run across a pause therefore + /// cannot use it, and only the host (which owns run persistence) knows an + /// id that survives. + /// + /// Today's consumer is the [`approval`](crate::nodes::integration::approval) + /// node, whose `request_id` defaults to `":"` — the key the + /// host's [`ApprovalProvider`](crate::caps::ApprovalProvider) de-duplicates + /// reviews on. Unique per run *and* stable across resume is exactly the + /// property that makes one human review, rather than a fresh card every + /// time the run is looked at. + /// + /// **Must be server-generated.** It lands in `run.id`, outside the + /// caller-supplied trigger payload, precisely so it is not attacker + /// influenced; copying a request field into it hands an attacker the + /// de-duplication key and, with it, an earlier run's approval. + /// + /// `None` leaves `run.id` unset, which is what every caller predating this + /// meant. + pub run_id: Option, } impl RunInput { @@ -156,6 +181,7 @@ impl RunInput { trigger, inputs: Map::new(), approvals: Vec::new(), + run_id: None, } } @@ -172,6 +198,17 @@ impl RunInput { self.approvals = approvals; self } + + /// Names this run with a durable, host-generated id (see [`Self::run_id`]). + /// + /// Pass the **same** id when resuming the run: that is what makes a paused + /// human review resolve to the one already in front of a person instead of + /// opening a second one. + #[must_use] + pub fn with_run_id(mut self, run_id: impl Into) -> Self { + self.run_id = Some(run_id.into()); + self + } } impl From for RunInput { diff --git a/src/engine/run_state.rs b/src/engine/run_state.rs index 3d86934..75bfe57 100644 --- a/src/engine/run_state.rs +++ b/src/engine/run_state.rs @@ -89,6 +89,7 @@ pub(super) async fn build_and_run( trigger, inputs, approvals, + run_id: host_run_id, } = input.into(); let resolved_inputs = crate::model::resolve_inputs(&workflow.graph.inputs, &inputs)?; @@ -125,6 +126,15 @@ pub(super) async fn build_and_run( // `=inputs.` (and jq programs walking `run` still see it too). let mut initial = json!({ "run": { "trigger": trigger, "inputs": resolved_inputs, "approvals": approvals } }); + // The host's durable run identity, seeded OUTSIDE `run.trigger` on purpose: + // the trigger is caller-supplied and, on a webhook, attacker-influenced, + // while this slot is one a host fills deliberately. Anything keyed on it + // (today the `approval` node's `request_id`) therefore rests on an id the + // host chose, not one a request could smuggle in. Absent when the caller + // named no run, which is every caller predating `RunInput::run_id`. + if let Some(host_run_id) = host_run_id { + merge(&mut initial, json!({ "run": { "id": host_run_id } })); + } merge(&mut initial, seed_items); // The nesting cap for `sub_workflow` chains, read off the trigger config // like every other run-level knob and seeded into the run state so the @@ -279,7 +289,7 @@ pub async fn resume( } /// Unions `newly_approved` into the run input's `trigger["approvals"]`, leaving -/// the declared-input values untouched. +/// the declared-input values and the run's host id untouched. /// /// Reads defensively: a missing or non-array `approvals` yields an empty /// starting set, and a non-object trigger (which carries no fields to preserve) @@ -295,6 +305,7 @@ pub(super) fn merge_approvals(input: impl Into, newly_approved: Vec, newly_approved: Vec>, +} + +impl SlowDesk { + fn new(answer_after: usize) -> Self { + Self { + answer_after, + calls: AtomicUsize::new(0), + seen: Mutex::new(Vec::new()), + } + } + + fn request_ids(&self) -> Vec { + self.seen.lock().expect("lock").clone() + } +} + +#[async_trait] +impl ApprovalProvider for SlowDesk { + async fn decide(&self, request: &ApprovalRequest) -> Result { + self.seen + .lock() + .expect("lock") + .push(request.request_id.clone()); + let seen = self.calls.fetch_add(1, Ordering::SeqCst) + 1; + if seen < self.answer_after { + return Ok(ApprovalOutcome::Pending); + } + Ok(ApprovalOutcome::Decided(ApprovalDecision { + approved: true, + decided_by: Some("editor".into()), + comment: Some("looks right".into()), + payload: None, + })) + } +} + +/// Records the order in which nodes finished, which is how this test tells +/// concurrency from mere eventual completion. +#[derive(Default)] +struct FinishOrder { + order: Mutex>, +} + +impl FinishOrder { + /// Where a node **first** appears. A polling node records a step per poll, + /// so for `review` this is the first look, not the verdict. + fn first_position_of(&self, node: &str) -> Option { + self.order + .lock() + .expect("lock") + .iter() + .position(|id| id == node) + } + + /// Where a node **last** appears — for a polling node, the activation that + /// actually settled it. + fn last_position_of(&self, node: &str) -> Option { + self.order + .lock() + .expect("lock") + .iter() + .rposition(|id| id == node) + } + + fn count_of(&self, node: &str) -> usize { + self.order + .lock() + .expect("lock") + .iter() + .filter(|id| *id == node) + .count() + } + + fn finished(&self) -> Vec { + self.order.lock().expect("lock").clone() + } +} + +impl RunObserver for FinishOrder { + fn on_step_finish(&self, step: &ExecutionStep) { + if matches!(step.status, StepStatus::Success) { + self.order.lock().expect("lock").push(step.node_id.clone()); + } + } +} + +fn node(id: &str, kind: NodeKind, config: Value) -> Node { + Node { + id: id.into(), + kind, + type_version: 1, + name: id.into(), + config, + ports: vec![], + position: None, + } +} + +fn edge(from: &str, port: &str, to: &str, to_port: &str) -> Edge { + Edge { + from_node: from.into(), + from_port: port.into(), + to_node: to.into(), + to_port: to_port.into(), + } +} + +/// trigger fans out to two branches: +/// +/// - `review` — an `approval` in poll mode, waiting on a human; +/// - `enrich -> score -> summarize` — independent work that must not wait. +/// +/// Both land on `combine`, a `merge` barrier that absorbs the verdict and the +/// work together. +fn graph() -> WorkflowGraph { + WorkflowGraph { + name: "async approval".into(), + nodes: vec![ + node("trigger", NodeKind::Trigger, Value::Null), + node( + "review", + NodeKind::Approval, + json!({ + "title": "Publish this?", + "subject_kind": "url", + "subject": "=item.url", + "wait_mode": "poll", + "poll_interval_ms": 1, + "max_polls": 50, + }), + ), + node( + "enrich", + NodeKind::Transform, + json!({ "set": { "url": "=item.url", "enriched": true } }), + ), + node( + "score", + NodeKind::Transform, + json!({ "set": { "url": "=item.url", "score": 91 } }), + ), + node( + "summarize", + NodeKind::Transform, + json!({ "set": { "url": "=item.url", "score": "=item.score", "ready": true } }), + ), + node("combine", NodeKind::Merge, json!({ "mode": "append" })), + ], + edges: vec![ + edge("trigger", "main", "review", "main"), + edge("trigger", "main", "enrich", "main"), + edge("enrich", "main", "score", "main"), + edge("score", "main", "summarize", "main"), + edge("review", "approved", "combine", "verdict"), + edge("summarize", "main", "combine", "work"), + ], + ..Default::default() + } +} + +#[tokio::test] +async fn a_polling_review_lets_the_rest_of_the_graph_run_and_then_absorbs_the_verdict() { + let compiled = compile(&graph()).expect("compile"); + // The human answers on the 4th look — long enough that the three-node work + // branch has somewhere to get to in the meantime. + let desk = Arc::new(SlowDesk::new(4)); + let caps = Capabilities { + approvals: Some(desk.clone()), + ..mock_capabilities() + }; + let order = Arc::new(FinishOrder::default()); + let observer: Arc = order.clone(); + + let outcome = tokio::time::timeout( + GUARD, + run_with_observer( + &compiled, + RunInput::new(json!({ "url": "https://example.com/drafts/42" })) + .with_run_id("run-async-1"), + &caps, + &observer, + ), + ) + .await + .expect("the run must not hang waiting on the review") + .expect("run"); + + // 1. The run completed on its own. Nothing paused: a polling review never + // interrupts, so the host was never asked to resume anything. + assert!( + outcome.pending_approvals.is_empty(), + "a polling review must not pause the run, got {:?}", + outcome.pending_approvals + ); + + // 2. The work branch ran to completion WHILE the review was outstanding. + // The finish order interleaves — review, enrich, review, score, review, + // summarize, review — because each poll is its own activation and the + // sibling branch advances in the super-steps between them. So the test + // is: the review was still being polled *before* the work started, and + // it settled *after* the work had finished. + let finished = order.finished(); + let first_review = order + .first_position_of("review") + .unwrap_or_else(|| panic!("the review must have been polled, saw {finished:?}")); + let settled_review = order + .last_position_of("review") + .unwrap_or_else(|| panic!("the review must have settled, saw {finished:?}")); + let enrich = order + .first_position_of("enrich") + .unwrap_or_else(|| panic!("the work branch must have started, saw {finished:?}")); + let summarize = order + .last_position_of("summarize") + .unwrap_or_else(|| panic!("the work branch must have finished, saw {finished:?}")); + + // This ordering is not a scheduler race: this test runs on the default + // `current_thread` tokio flavor (one poller, no cross-thread scheduling), + // and neither `review`'s first poll (`SlowDesk::decide`, a synchronous + // `std::sync::Mutex` push with no real I/O) nor `enrich` (a synchronous + // jq transform) suspends on its first poll. `run_active_parallel` + // (src/graph/compiled/executor/node_execution.rs) drives the two + // trigger-fanned-out branches via `futures_util::future::join_all`, which + // polls each branch's future in the order it was pushed — `review`'s edge + // is declared before `enrich`'s in `graph()` above — so `review` runs to + // completion, including firing `on_step_finish`, before `enrich` is ever + // polled. + assert!( + first_review < enrich, + "the review must already be outstanding when the work branch starts — \ + finish order was {finished:?}" + ); + assert!( + summarize < settled_review, + "the work branch must finish before the verdict lands, not after it — \ + finish order was {finished:?}" + ); + assert!( + order.count_of("review") > 1, + "the review must have been polled repeatedly rather than settling at once — \ + finish order was {finished:?}" + ); + + // 3. The human was asked more than once, always about the SAME review. + // A polling loop that re-registered the request would notify a person + // once per poll; create-or-fetch on `request_id` is what prevents that. + let ids = desk.request_ids(); + assert!( + ids.len() >= 2, + "the review should have been outstanding across several polls, got {ids:?}" + ); + assert!( + ids.iter().all(|id| id == "run-async-1:review"), + "every poll must address one review, got {ids:?}" + ); + + // 4. The graph absorbed the verdict: the merge carries both the decision + // and the work the branch did while waiting for it. + let merged = outcome.output["nodes"]["combine"]["items"] + .as_array() + .expect("the merge emitted items"); + let jsons: Vec<&Value> = merged.iter().map(|item| &item["json"]).collect(); + assert!( + jsons.iter().any(|json| json["approved"] == json!(true)), + "the verdict must reach the merge, got {jsons:?}" + ); + assert!( + jsons.iter().any(|json| json["ready"] == json!(true)), + "the work done while waiting must reach the merge, got {jsons:?}" + ); +} + +/// The same shape, but the human says no: the graph still ran its independent +/// work, and the rejection routes to its own branch rather than failing the run. +#[tokio::test] +async fn a_rejection_arriving_late_routes_without_disturbing_the_work_branch() { + struct RejectingDesk; + + #[async_trait] + impl ApprovalProvider for RejectingDesk { + async fn decide(&self, _request: &ApprovalRequest) -> Result { + Ok(ApprovalOutcome::Decided(ApprovalDecision::rejected(Some( + "wrong draft".into(), + )))) + } + } + + let mut graph = graph(); + graph.nodes.push(node( + "revise", + NodeKind::Transform, + json!({ "set": { "revise_because": "=item.comment" } }), + )); + graph + .edges + .push(edge("review", "rejected", "revise", "main")); + + let compiled = compile(&graph).expect("compile"); + let caps = Capabilities { + approvals: Some(Arc::new(RejectingDesk)), + ..mock_capabilities() + }; + + let outcome = tokio::time::timeout( + GUARD, + run_with_observer( + &compiled, + RunInput::new(json!({ "url": "https://example.com/drafts/42" })) + .with_run_id("run-async-2"), + &caps, + &(Arc::new(tinyflows::observability::NoopObserver) as Arc), + ), + ) + .await + .expect("the run must not hang") + .expect("run"); + + assert_eq!(outcome.output["nodes"]["review"]["port"], "rejected"); + assert_eq!( + outcome.output["nodes"]["revise"]["items"][0]["json"]["revise_because"], "wrong draft", + "the rejection branch runs with the reviewer's reason" + ); + assert_eq!( + outcome.output["nodes"]["summarize"]["items"][0]["json"]["ready"], + json!(true), + "the independent work branch still completed" + ); +} diff --git a/wiki/Capability-Traits.md b/wiki/Capability-Traits.md index bae43f3..257b119 100644 --- a/wiki/Capability-Traits.md +++ b/wiki/Capability-Traits.md @@ -52,6 +52,16 @@ example, durable key/value state via `ctx.caps.state`, and a human review via must add an `approvals` field (`None` if it wires no provider) to keep compiling. +An `approval` node needs an identity for its review: either an explicit +`request_id` in its own config, or — the common case, since a graph is +authored once and run many times — `RunInput::with_run_id("...")` on the run, +which the node falls back to as `":"`. A node with neither +has nothing stable to key the review on and refuses to run rather than +guessing; it does not silently fall back to the bare node id, which would let +a later run of the same graph reuse an earlier run's decision. Use a +**server-generated** run id, never a caller-supplied field — it is the key +reviews are de-duplicated on. + `ApprovalProvider::decide` is **create-or-fetch**, keyed on `ApprovalRequest::request_id`: the first call with an id creates the review, and every later call with that id reports where *that* review stands. This matters