diff --git a/src/openhuman/agent/harness/subagent_runner/ops/provider.rs b/src/openhuman/agent/harness/subagent_runner/ops/provider.rs index b43b9dcbba..b55284331a 100644 --- a/src/openhuman/agent/harness/subagent_runner/ops/provider.rs +++ b/src/openhuman/agent/harness/subagent_runner/ops/provider.rs @@ -252,8 +252,9 @@ pub(crate) struct LazyToolkitResolver { /// for a slug, subsequent `resolve()` calls for the same slug reuse the /// cached instance — sharing its [`ContractGate`] state (#5119). #[allow(dead_code)] // used via pub(super) from tests - pub(super) resolved: - std::sync::Mutex>>, + pub(super) resolved: std::sync::Mutex< + std::collections::HashMap>, + >, } /// Minimum normalized-slug length before the prefix/superstring tier in @@ -302,14 +303,13 @@ impl LazyToolkitResolver { } let action = self.find_action(name)?; - let tool: std::sync::Arc = std::sync::Arc::new( - crate::openhuman::composio::ComposioActionTool::new( + let tool: std::sync::Arc = + std::sync::Arc::new(crate::openhuman::composio::ComposioActionTool::new( self.config.clone(), action.name.clone(), action.description.clone(), action.parameters.clone(), - ), - ); + )); // Store in cache for future lookups. { diff --git a/src/openhuman/approval/redact.rs b/src/openhuman/approval/redact.rs index b457ec028e..e3712f7293 100644 --- a/src/openhuman/approval/redact.rs +++ b/src/openhuman/approval/redact.rs @@ -69,6 +69,21 @@ const SENSITIVE_KEYS: &[&str] = &[ "authorization", "auth", "code", + // File-handoff params. A presigned storage link (`storage_get_link`) is a + // BEARER CAPABILITY: anyone holding the URL can fetch the file until it + // expires. These land in `tool_call` args whenever a flow hands a produced + // file to an externally-executed action (a Composio `file_uploadable` + // param such as Gmail's `attachment` or Jira's `file_to_upload`). Redacted + // args are both rendered on the approval card and persisted with the + // approval record, so leaving these clear would leak the capability into + // the UI and durable storage. + "attachment", + "attachments", + "file_to_upload", + "file_url", + "public_url", + "signed_url", + "presigned_url", ]; /// Produce a redacted clone of `args` suitable for persistence / @@ -485,6 +500,41 @@ mod tests { assert_eq!(summary.matches("").count(), 2); } + #[test] + fn file_handoff_links_are_redacted() { + // A presigned storage link is a bearer capability: anyone holding the + // URL can fetch the file until it expires. Redacted args are shown on + // the approval card AND persisted, so these must never appear clear. + let args = json!({ + "attachment": "https://files.example.test/f_1?sig=SECRETSIGNATURE", + "file_to_upload": "https://files.example.test/f_2?sig=ANOTHERSIG", + "file_url": "https://files.example.test/f_3?sig=THIRDSIG", + "public_url": "https://files.example.test/f_4?sig=FOURTHSIG", + // A bare `url` (e.g. an `http_request` node's destination, or a + // Composio action's benign url arg) is NOT a file-handoff key and + // MUST stay visible so the human approver can judge the action. + "url": "https://webhook.site/VISIBLE-DESTINATION", + }); + let red = redact_args(&args); + let blob = red.to_string(); + for leaked in [ + "SECRETSIGNATURE", + "ANOTHERSIG", + "THIRDSIG", + "FOURTHSIG", + "files.example.test", + ] { + assert!( + !blob.contains(leaked), + "presigned link leaked through redaction ({leaked}): {blob}" + ); + } + assert!( + blob.contains("webhook.site/VISIBLE-DESTINATION"), + "a bare `url` (e.g. an http_request destination) must NOT be redacted: {blob}" + ); + } + #[test] fn summarize_action_pulls_safe_fields() { let args = json!({ diff --git a/src/openhuman/flows/agents/workflow_builder/prompt.md b/src/openhuman/flows/agents/workflow_builder/prompt.md index 3dd5028101..5f7c0e3daf 100644 --- a/src/openhuman/flows/agents/workflow_builder/prompt.md +++ b/src/openhuman/flows/agents/workflow_builder/prompt.md @@ -551,6 +551,65 @@ And without `input_context`, don't reach for a jq expression woven into — that's prose, not jq, resolves to `null`, and both the `save_workflow` gate and `dry_run_workflow`'s `agent_prompt_nulls` will reject it. +### Attaching a produced file to an external action + +When the user wants a file **attached** to something you send or create through +a connected integration, the file must be handed over as a **URL the provider's +servers can fetch**. Composio actions execute on Composio's backend, so a local filesystem +path can never work: it fails at run time with `Error reading file at +/Users/... ENOENT`. Putting the content in the message body does **not** +satisfy an attachment request either. + +The working chain is four nodes. **Keep the file handle out of any agent's +structured output.** Give the producer a fixed path you choose, then refer to +that same literal path from a separate upload node. An agent that has to emit a +`file_id` or `file_path` through `output_parser` is a schema mismatch waiting to +fail the run; the file only needs to exist on disk, so the agent's *side effect* +is what matters, not its output. + +1. **Produce the file at a workspace-relative path YOU chose.** An `agent` node + (usually `agent_ref: "code_executor"`) or a `code` node writes it. State the + path in the prompt, e.g. "write the page to `report.html`". Use a path + **relative to the working directory**, never an absolute path like + `/tmp/...`: writes and uploads are confined to the agent workspace, and an + absolute path outside it is rejected. Do **not** give this node an + `output_parser` schema for the file, and do **not** bind anything off it. +2. **Upload it.** A `tool_call` on **`oh:storage_upload_file`** with that same + path as a **literal** string: `{ "path": "report.html" }`. Because this is a + real node, its `file_id` is a node output rather than model-authored JSON: + bind `=nodes..item.json.file_id`. +3. **Mint a link that outlives approval.** A `tool_call` on + **`oh:storage_get_link`** with + `{ "file_id": "=nodes..item.json.file_id", "expires_in_seconds": 900 }` + returns `{ url, expires_at }`. Bind `=nodes..item.json.url`. + The send is an outbound action, so it may be parked for human approval for up + to ~10 minutes before it fires; the link's TTL must comfortably outlive that + window or the provider will fetch a dead URL. The URL is a bearer capability + for as long as it lives, so do not set it far longer than needed either. + **Do not** upload with `visibility: "public"` to get a `public_url` instead. + That leaves a permanently world-readable object; the presigned link expires. +4. **Send it.** A `tool_call` on the provider action, binding the link URL into + that action's file parameter. + +**Find the file parameter by its marker, never by guessing a name.** Call +`get_tool_contract` on the send action and look in `input_schema.properties` +for the property carrying **`"file_uploadable": true`** (it also shows +`"format": "path"`). That marker is the contract across toolkits; the property +name is not, so read it off the contract every time rather than assuming a +convention (one toolkit's is `attachment`, another's is `file_to_upload`). + +Everything else about that parameter also comes from the contract: whether it +accepts one value or a list, and any size or type limits the provider enforces, +are stated in its schema and `description`. Read them there. Do not rely on +remembered provider limits. + +**Do not invent a dedicated attachment action.** Send actions generally take +the file on the ordinary send, so search for an attachment-capable send before +assuming a separate one exists. If `get_tool_contract` reports a slug is not a +real action, that is a hard stop: go back to `search_tool_catalog` and pick a +real one rather than wiring it anyway. A slug that merely looks plausible by +naming convention is the single most expensive mistake you can make here. + ### Trigger kinds — which ones actually fire Set `config.trigger_kind` on the trigger node. **Only three fire automatically diff --git a/src/openhuman/flows/builder_tools.rs b/src/openhuman/flows/builder_tools.rs index 170c3c0bc0..2f756f5a0e 100644 --- a/src/openhuman/flows/builder_tools.rs +++ b/src/openhuman/flows/builder_tools.rs @@ -2587,32 +2587,52 @@ impl Tool for GetNodeKindContractTool { /// /// The common case reports `{ node_id, location, expression }` — a wiring /// mistake the agent should fix. But when the null-resolved expression binds to -/// the output of an upstream Composio `tool_call` node -/// ([`ops::composio_tool_call_upstream_ref`]), the entry is instead marked +/// the output of an upstream Composio-or-native `tool_call` node +/// ([`ops::mock_opaque_tool_call_upstream_ref`]), the entry is instead marked /// `unverifiable: true` and carries an honest `suggestion`: the echo sandbox -/// can NEVER produce a Composio tool's real output fields, so this particular -/// null is expected here and does NOT prove the binding wrong (WS6 — the -/// transcript audit where the agent re-wired an already-correct binding three -/// times chasing this exact false negative). The message points at -/// `get_tool_contract` / `get_tool_output_sample` as the real disambiguators. +/// can NEVER produce a tool's real output fields, so this particular null is +/// expected here and does NOT prove the binding wrong (WS6 — the transcript +/// audit where the agent re-wired an already-correct binding three times +/// chasing this exact false negative). The suggestion adapts to the upstream +/// kind: a Composio upstream points at `get_tool_contract` / +/// `get_tool_output_sample` and the `.item.json.data.` nesting; a native `oh:` +/// upstream points at the flat `.item.json.` shape instead. fn build_null_resolution_entry( node_id: &str, diag: &tinyflows::expr::NullResolution, graph: &WorkflowGraph, ) -> Value { - if let Some(upstream) = crate::openhuman::flows::ops::composio_tool_call_upstream_ref( + if let Some(upstream) = crate::openhuman::flows::ops::mock_opaque_tool_call_upstream_ref( &diag.expression, graph, node_id, ) { let field = diag.location.strip_prefix("args.").unwrap_or("args"); - return json!({ - "node_id": node_id, - "location": diag.location, - "expression": diag.expression, - "unverifiable": true, - "upstream_tool_call": upstream, - "suggestion": format!( + // The disambiguation advice differs by upstream kind: a native `oh:` + // tool's output binds FLAT (`.item.json.`) after + // `native_tool_payload`'s unwrap — it has no `.data.` wrapper and no + // Composio `get_tool_contract` — whereas a Composio action nests under + // `.item.json.data.`. Emitting the Composio advice for a native + // upstream would send the agent chasing a `.data.` path that will + // never exist. + let upstream_is_native = graph + .nodes + .iter() + .find(|n| n.id == upstream) + .and_then(|n| n.config.get("slug").and_then(Value::as_str)) + .is_some_and(|s| s.starts_with("oh:")); + let suggestion = if upstream_is_native { + format!( + "required arg `{field}` binds to the output of native tool_call node \ + `{upstream}` — the SANDBOX only echoes tool calls and can never produce \ + their real output fields, so this binding is UNVERIFIABLE here (not \ + necessarily wrong). A native `oh:` tool's real output binds FLAT at \ + `=nodes.{upstream}.item.json.` (no `.data.` wrapper). Confirm the \ + field name against that tool's own output shape. It is a real bug only if \ + the path doesn't match the tool's actual output." + ) + } else { + format!( "required arg `{field}` binds to the output of Composio tool_call node \ `{upstream}` — the SANDBOX only echoes tool calls and can never produce \ their real output fields, so this binding is UNVERIFIABLE here (not \ @@ -2621,7 +2641,15 @@ fn build_null_resolution_entry( `.item.json.data.`), or get_tool_output_sample {{ slug, args }} for the \ real shape. It is a real bug only if the path doesn't match the action's \ actual output." - ), + ) + }; + return json!({ + "node_id": node_id, + "location": diag.location, + "expression": diag.expression, + "unverifiable": true, + "upstream_tool_call": upstream, + "suggestion": suggestion, }); } json!({ @@ -2634,7 +2662,7 @@ fn build_null_resolution_entry( /// Every null-resolved `args.*` config expression that landed on a `tool_call` /// node, as `null_resolutions` diagnostic entries (see /// [`build_null_resolution_entry`] for the shape, including the WS6 -/// `unverifiable` Composio-upstream variant). Shared by the settled-run path +/// `unverifiable` Composio-or-native-upstream variant). Shared by the settled-run path /// (which fails the dry run on these) and the errored-run path (which surfaces /// only the `unverifiable` ones so a stop-policy preflight abort explains /// itself honestly instead of via the generic required-arg text). diff --git a/src/openhuman/flows/ops.rs b/src/openhuman/flows/ops.rs index 97fcff5d6a..c11880c52e 100644 --- a/src/openhuman/flows/ops.rs +++ b/src/openhuman/flows/ops.rs @@ -442,6 +442,57 @@ pub(crate) fn to_flow_validation_error( /// Assumes `graph` is already structurally valid (run /// `validate_and_migrate_graph` / `validate_all` first) — these gates check /// resolvability/contracts on a compilable graph. +/// +/// Author-gate for `oh:storage_upload_file`: its literal `path` arg must be +/// workspace-relative. Uploads are confined to the agent workspace by the +/// runtime `resolve_upload_path` (a canonicalized path that escapes `action_dir` +/// is rejected), so an absolute path like `/tmp/report.html` or one climbing out +/// with `..` cannot work — it fails mid-run at the upload step. The prompt tells +/// the builder to use a relative path, but the model reliably ignores that and +/// copies an absolute path from a prior flow's example, so this enforces it in +/// code (a hard, actionable author-gate) rather than trusting the prose. +/// +/// Only LITERAL paths are checked: a `=`-expression resolves from upstream data +/// at runtime and is out of scope here (the runtime check still applies). An +/// absent `path` is left to the required-arg gate. +pub(crate) fn validate_upload_paths(graph: &WorkflowGraph) -> Vec { + const UPLOAD_SLUG: &str = "oh:storage_upload_file"; + let mut errors = Vec::new(); + for node in &graph.nodes { + if node.kind != NodeKind::ToolCall { + continue; + } + if node.config.get("slug").and_then(Value::as_str) != Some(UPLOAD_SLUG) { + continue; + } + let Some(raw) = node + .config + .get("args") + .and_then(|a| a.get("path")) + .and_then(Value::as_str) + else { + continue; + }; + let path = raw.trim(); + // Dynamic (resolved at runtime) or absent — not a literal we can check here. + if path.is_empty() || path.starts_with('=') { + continue; + } + let escapes_via_parent = path.split(['/', '\\']).any(|seg| seg == ".."); + if std::path::Path::new(path).is_absolute() || escapes_via_parent { + errors.push(format!( + "Node '{}': `oh:storage_upload_file` path `{path}` must be workspace-relative \ + (e.g. `report.html`). Uploads are confined to the agent workspace, so an \ + absolute path (`/tmp/...`, `/Users/...`) or one escaping with `..` is rejected \ + at run time. Use a relative path, and have the producing node write the file to \ + that same relative path.", + node.id + )); + } + } + errors +} + pub(crate) async fn run_builder_gates(config: &Config, graph: &WorkflowGraph) -> Vec { let compatibility_errors = config_aware_engine_compatibility_errors(config, graph); if !compatibility_errors.is_empty() { @@ -452,6 +503,14 @@ pub(crate) async fn run_builder_gates(config: &Config, graph: &WorkflowGraph) -> if !binding_errors.is_empty() { return binding_errors; } + // Cheap, sync: an `oh:storage_upload_file` literal `path` that is absolute or + // escapes the workspace. The runtime `resolve_upload_path` rejects it, but the + // model reliably ignores the prompt's "use a workspace-relative path" rule and + // copies an absolute `/tmp/...` path from prior flows, so enforce it in code. + let upload_path_errors = validate_upload_paths(graph); + if !upload_path_errors.is_empty() { + return upload_path_errors; + } // Cheap: an `agent` node's `agent_ref` that would hit the runtime's // `RegistryFallback` "unknown agent_ref" hard error mid-run. Almost always a // pure in-memory harness-registry lookup; only a ref that ISN'T a harness @@ -2371,19 +2430,21 @@ pub(crate) async fn validate_required_arg_resolvability(graph: &WorkflowGraph) - ); continue; } - // A null bound to the OUTPUT of an upstream Composio `tool_call` - // node is UNVERIFIABLE in this echo sandbox — the mock renders a - // Composio `tool_call` as `{tool, args, connection}` and can NEVER - // produce its real output fields (`.item.json.data.`), so a - // downstream binding to one resolves `null` here even when the - // wiring is perfectly correct. Hard-rejecting it (WS6) would block - // a possibly-correct graph from ever being proposed — the exact - // false-negative the transcript audit caught. Downgrade to a - // debug-logged skip; `dry_run_workflow` remains the surface that - // reports it (as an `unverifiable` diagnostic the agent can act on - // via get_tool_contract / get_tool_output_sample). + // A null bound to the OUTPUT of an upstream Composio-or-native + // `tool_call` node is UNVERIFIABLE in this echo sandbox — the mock + // renders BOTH a Composio and a native `oh:` `tool_call` as + // `{tool, args, connection}` and can NEVER produce their real output + // fields (`.item.json.data.` for Composio, `.item.json.` + // for a native tool), so a downstream binding to one resolves `null` + // here even when the wiring is perfectly correct. Hard-rejecting it + // (WS6) would block a possibly-correct graph from ever being proposed + // — the exact false-negative the transcript audit caught, and the one + // that made this gate reject #5148's own native-attachment chain. + // Downgrade to a debug-logged skip; `dry_run_workflow` remains the + // surface that reports it (as an `unverifiable` diagnostic the agent + // can act on via get_tool_contract / get_tool_output_sample). if let Some(upstream) = - composio_tool_call_upstream_ref(&diag.expression, graph, &step.node_id) + mock_opaque_tool_call_upstream_ref(&diag.expression, graph, &step.node_id) { tracing::debug!( target: "flows", @@ -2392,7 +2453,7 @@ pub(crate) async fn validate_required_arg_resolvability(graph: &WorkflowGraph) - %field, upstream = %upstream, expression = %diag.expression, - "[flows] required-arg resolvability check: arg binds to a Composio \ + "[flows] required-arg resolvability check: arg binds to a Composio-or-native \ tool_call's output — UNVERIFIABLE in the echo sandbox (the mock cannot \ produce real tool output fields), not rejecting; dry_run_workflow \ reports it instead" @@ -2515,18 +2576,25 @@ fn is_trigger_scoped_expression( } /// If a null-resolved config expression on `node_id` is bound to the OUTPUT of -/// an upstream **Composio `tool_call`** node (a `tool_call` whose `slug` is a -/// real Composio action — not `=`-derived, not native `oh:`), returns that -/// upstream node's id; otherwise `None`. +/// an upstream **`tool_call`** node whose sandbox output is an opaque echo — a +/// Composio curated action OR a native `oh:` tool (anything but a `=`-derived +/// dynamic slug) — returns that upstream node's id; otherwise `None`. +/// +/// The dry-run / gate sandbox renders BOTH a Composio `tool_call` and a native +/// `oh:` `tool_call` as a deterministic echo (`{tool, args, connection}`) and +/// can NEVER produce their real output fields, so a downstream binding off such +/// a node (`.item.json.data.` for Composio, or `.item.json.` for +/// a native tool after `native_tool_payload`'s unwrap) resolves `null` in the +/// sandbox **even when the wiring is correct** — the binding is UNVERIFIABLE +/// here, not necessarily broken. Callers use this to tell that honest- +/// uncertainty case apart from a genuinely broken binding (one wired to an +/// `agent` / `transform` / `code` / trigger upstream, whose real output the +/// sandbox DOES produce, so a null there IS a real bug). /// -/// The dry-run / gate sandbox renders a Composio `tool_call` as a deterministic -/// echo (`{tool, args, connection}`) and can NEVER produce its real output -/// fields, so a downstream binding to `.item.json.data.` off such a node -/// resolves `null` in the sandbox **even when the wiring is correct** — the -/// binding is UNVERIFIABLE here, not necessarily broken. Callers use this to -/// tell that honest-uncertainty case apart from a genuinely broken binding -/// (one wired to an `agent` / `transform` / `code` / trigger upstream, whose -/// real output the sandbox DOES produce, so a null there IS a real bug). +/// The native `oh:` case is why this exists beyond Composio: #5148's guidance +/// prescribes a `produce -> oh:storage_upload_file -> oh:storage_get_link -> +/// send` chain where the send binds `=nodes.get_link.item.json.url`; excluding +/// native upstreams here made the gate hard-reject that exact (correct) chain. /// /// Handles both addressing forms the engine can trace: /// - explicit `=nodes....` / `=.nodes[""]...` (parsed via @@ -2536,9 +2604,9 @@ fn is_trigger_scoped_expression( /// ambiguous fan-in is never mis-attributed to a single upstream node. /// /// Anything else (a `=run...` trigger reference, a jq expression not rooted at -/// one of the above, or a reference to a non-`tool_call` / native / dynamic -/// node) returns `None`. -pub(crate) fn composio_tool_call_upstream_ref<'a>( +/// one of the above, or a reference to a non-`tool_call` / `=`-dynamic node) +/// returns `None`. +pub(crate) fn mock_opaque_tool_call_upstream_ref<'a>( expr: &str, graph: &'a WorkflowGraph, node_id: &str, @@ -2574,7 +2642,11 @@ pub(crate) fn composio_tool_call_upstream_ref<'a>( return None; } let slug = node.config.get("slug").and_then(Value::as_str)?; - if slug.starts_with('=') || slug.starts_with("oh:") { + // A `=`-derived slug is a dynamic runtime slug we can't reason about. But a + // native `oh:` tool_call IS opaque-echoed by the mock exactly like a + // Composio one, so its downstream null is equally unverifiable, not broken — + // do NOT exclude it (that exclusion made the gate reject #5148's own chain). + if slug.starts_with('=') { return None; } Some(node.id.as_str()) diff --git a/src/openhuman/flows/ops_tests.rs b/src/openhuman/flows/ops_tests.rs index 70b25bcdb8..1f00d6e5bd 100644 --- a/src/openhuman/flows/ops_tests.rs +++ b/src/openhuman/flows/ops_tests.rs @@ -3718,6 +3718,155 @@ async fn validate_required_arg_resolvability_ignores_native_and_dynamic_slugs() assert!(errors.is_empty(), "{errors:?}"); } +#[tokio::test] +async fn mock_opaque_tool_call_upstream_ref_matches_native_and_composio_upstreams() { + // Both a Composio curated action and a native `oh:` tool are opaque-echoed + // by the mock sandbox, so a null bound to EITHER is unverifiable (Some). + // An `agent` / `code` upstream's real output IS produced by the sandbox, and + // a `=`-dynamic slug is unknowable, so a null bound to those is genuine (None). + let g = graph(json!({ + "nodes": [ + { "id": "t", "kind": "trigger", "name": "Manual" }, + { "id": "code_up", "kind": "code", "name": "Code", + "config": { "language": "javascript", "source": "return {};" } }, + { "id": "agent_up", "kind": "agent", "name": "Agent", + "config": { "agent_ref": "researcher", "prompt": "x" } }, + { "id": "native_up", "kind": "tool_call", "name": "Link", + "config": { "slug": "oh:storage_get_link", "args": { "file_id": "f" } } }, + { "id": "composio_up", "kind": "tool_call", "name": "Profile", + "config": { "slug": "GMAIL_GET_PROFILE", "args": {} } }, + { "id": "dyn_up", "kind": "tool_call", "name": "Dyn", + "config": { "slug": "=item.slug", "args": {} } }, + { "id": "sink", "kind": "tool_call", "name": "Sink", + "config": { "slug": "GMAIL_SEND_EMAIL", "args": {} } } + ], + "edges": [] + })); + let up = |expr: &str| mock_opaque_tool_call_upstream_ref(expr, &g, "sink").map(str::to_string); + assert_eq!( + up("=nodes.native_up.item.json.url").as_deref(), + Some("native_up") + ); + assert_eq!( + up("=nodes.composio_up.item.json.data.emailAddress").as_deref(), + Some("composio_up") + ); + assert_eq!(up("=nodes.agent_up.item.json.field"), None); + assert_eq!(up("=nodes.code_up.item.json.field"), None); + assert_eq!(up("=nodes.dyn_up.item.json.x"), None); +} + +#[tokio::test] +async fn validate_required_arg_resolvability_downgrades_null_from_native_tool_call_upstream() { + // #5148's chain: a Composio `send` binds its `attachment` to a native + // `oh:storage_get_link` node's `url`. That `url` is null in the echo sandbox + // (native tools are opaque-echoed), but the wiring is correct, so the gate + // must NOT reject it. Before the native-upstream carve-out it did — the loop + // that halted the live "fix with agent" self-repair. + let g = graph(json!({ + "nodes": [ + { "id": "t", "kind": "trigger", "name": "Manual" }, + { "id": "prep", "kind": "code", "name": "Prep", + "config": { "language": "javascript", "source": "return {};" } }, + { "id": "get_link", "kind": "tool_call", "name": "Link", + "config": { "slug": "oh:storage_get_link", "args": { "file_id": "f_1" } } }, + { "id": "send", "kind": "tool_call", "name": "Send", + "config": { "slug": "GMAIL_SEND_EMAIL", + "args": { "recipient_email": "a@b.com", "subject": "hi", "body": "there", + "attachment": "=nodes.get_link.item.json.url" } } } + ], + "edges": [ + { "from_node": "t", "to_node": "prep" }, + { "from_node": "prep", "to_node": "get_link" }, + { "from_node": "get_link", "to_node": "send" } + ] + })); + let errors = validate_required_arg_resolvability(&g).await; + assert!( + errors.is_empty(), + "a native-upstream attachment null must be downgraded, got: {errors:?}" + ); +} + +#[tokio::test] +async fn native_file_attachment_chain_passes_required_arg_resolvability() { + // Drift check that was missing pre-merge: author #5148's OWN documented + // `produce -> oh:storage_upload_file -> oh:storage_get_link -> send` chain + // and assert the null-arg gate (the exact gate that rejected it in the live + // "fix with agent" loop) now passes it. Targets `validate_required_arg_ + // resolvability` directly (deterministic, no live catalog) rather than + // `run_builder_gates`, whose connection/contract gates need live Composio. + let g = graph(json!({ + "nodes": [ + { "id": "t", "kind": "trigger", "name": "Manual" }, + { "id": "make_page", "kind": "code", "name": "Write", + "config": { "language": "javascript", "source": "return {};" } }, + { "id": "upload", "kind": "tool_call", "name": "Upload", + "config": { "slug": "oh:storage_upload_file", "args": { "path": "report.html" } } }, + { "id": "get_link", "kind": "tool_call", "name": "Link", + "config": { "slug": "oh:storage_get_link", + "args": { "file_id": "=nodes.upload.item.json.file_id", "expires_in_seconds": 900 } } }, + { "id": "send", "kind": "tool_call", "name": "Send", + "config": { "slug": "GMAIL_SEND_EMAIL", + "args": { "recipient_email": "a@b.com", "subject": "AI trends", "body": "attached", + "attachment": "=nodes.get_link.item.json.url" } } } + ], + "edges": [ + { "from_node": "t", "to_node": "make_page" }, + { "from_node": "make_page", "to_node": "upload" }, + { "from_node": "upload", "to_node": "get_link" }, + { "from_node": "get_link", "to_node": "send" } + ] + })); + let errors = validate_required_arg_resolvability(&g).await; + assert!( + errors.is_empty(), + "the documented native attachment chain must pass the null-arg gate, got: {errors:?}" + ); +} + +fn upload_graph(path: Value) -> WorkflowGraph { + graph(json!({ + "nodes": [ + { "id": "t", "kind": "trigger", "name": "Manual" }, + { "id": "up", "kind": "tool_call", "name": "Upload", + "config": { "slug": "oh:storage_upload_file", "args": { "path": path } } } + ], + "edges": [ { "from_node": "t", "to_node": "up" } ] + })) +} + +#[test] +fn validate_upload_paths_rejects_an_absolute_path() { + // The live-observed bug: the model copies `/tmp/openhuman-flow/report.html` + // from a prior flow, which the runtime rejects (uploads are confined to the + // workspace). Catch it at author time with an actionable message. + let errors = validate_upload_paths(&upload_graph(json!("/tmp/openhuman-flow/report.html"))); + assert_eq!(errors.len(), 1, "{errors:?}"); + assert!(errors[0].contains("'up'"), "{}", errors[0]); + assert!(errors[0].contains("workspace-relative"), "{}", errors[0]); +} + +#[test] +fn validate_upload_paths_accepts_a_workspace_relative_path() { + assert!(validate_upload_paths(&upload_graph(json!("report.html"))).is_empty()); + assert!(validate_upload_paths(&upload_graph(json!("out/report.html"))).is_empty()); +} + +#[test] +fn validate_upload_paths_rejects_a_parent_escape() { + let errors = validate_upload_paths(&upload_graph(json!("../../etc/passwd"))); + assert_eq!(errors.len(), 1, "{errors:?}"); + assert!(errors[0].contains("escaping with `..`"), "{}", errors[0]); +} + +#[test] +fn validate_upload_paths_ignores_a_dynamic_path_expression() { + // A `=`-expression resolves at runtime; the author-gate can't know its value, + // so it must not reject it (the runtime check still applies). + assert!(validate_upload_paths(&upload_graph(json!("=nodes.prep.item.json.path"))).is_empty()); +} + /// (Codex feedback on PR #4826) This gate sandbox-runs every graph against /// `json!({})` as the trigger payload, so a `tool_call` arg wired straight to /// the trigger's own data — `"to": "=item.email"` on a node whose only diff --git a/src/openhuman/tinyflows/caps.rs b/src/openhuman/tinyflows/caps.rs index c67e0b954c..985ebad9fa 100644 --- a/src/openhuman/tinyflows/caps.rs +++ b/src/openhuman/tinyflows/caps.rs @@ -442,7 +442,10 @@ fn extract_fenced_json_block(text: &str) -> Option { let fence_start = text.find("```")?; let after_fence = text[fence_start + 3..].trim(); // Skip optional "json" after the opening fence - let content = after_fence.strip_prefix("json").unwrap_or(after_fence).trim(); + let content = after_fence + .strip_prefix("json") + .unwrap_or(after_fence) + .trim(); // Find the *last* closing ``` (preferring the outermost fence, which // matches how Markdown renderers treat nested fences — the last ``` is // the one that closes the block the LLM opened). @@ -2722,6 +2725,63 @@ fn reject_unsuccessful_composio_response( ))) } +/// Native-tool analogue of [`reject_unsuccessful_composio_response`]. +/// +/// `execute_tool` returns `Ok(outcome)` for a tool that *ran* but *failed* — +/// the failure rides on [`ToolResult::is_error`] (quota exceeded, file missing, +/// no integration client configured). Nothing downstream inspected that flag, +/// so the tinyflows engine recorded the step — and therefore the run — as +/// `Success` even though the tool never did its job. Concretely: a file-upload +/// step could fail, the next node would bind a `null` URL, and the run still +/// reported "completed". +/// +/// Mirrors the Composio branch's contract so both paths turn a failed step into +/// `StepStatus::Error` (and, via `degrade_completed_status`, a failed run) +/// rather than a false "Completed". +fn reject_failed_native_tool_result( + slug: &str, + result: &crate::openhuman::skills::types::ToolResult, +) -> Result<()> { + if !result.is_error { + return Ok(()); + } + let rendered = result.output(); + let detail = match rendered.trim() { + "" => "no error detail returned by the tool", + d => d, + }; + tracing::warn!( + target: "flows", + %slug, + %detail, + "[flows] tool_call: native tool reported is_error — failing the step" + ); + Err(EngineError::Capability(format!( + "tool_call `{slug}` failed: {detail}" + ))) +} + +/// Unwraps a native (`oh:`) tool's [`ToolResult`] into the value a downstream +/// node actually binds against. +/// +/// Serializing the `ToolResult` verbatim (the previous behavior) placed the +/// whole envelope on `item.json`, so reaching a field required +/// `=nodes..item.json.content[0].data.`. That expression does +/// evaluate, but no builder agent ever emits it, which left native tools +/// effectively unbindable in practice. +/// +/// A lone `Json` block therefore returns its `data` directly, so a native node +/// binds with the same `=nodes..item.json.` shape used everywhere +/// else. Anything else (plain text, or mixed/multiple blocks) collapses to +/// `{ "text": }` so there is always a predictable field to bind. +fn native_tool_payload(result: &crate::openhuman::skills::types::ToolResult) -> Value { + use crate::openhuman::skills::types::ToolContent; + match result.content.as_slice() { + [ToolContent::Json { data }] => data.clone(), + _ => json!({ "text": result.output() }), + } +} + /// A [`ToolInvoker`] decorator that runs the host's Composio required-arg /// preflight before delegating to `inner`. /// @@ -2792,9 +2852,8 @@ impl ToolInvoker for OpenHumanTools { ) .await .map_err(EngineError::Capability)?; - return serde_json::to_value(&outcome.result).map_err(|e| { - EngineError::Capability(format!("could not serialize tool result: {e}")) - }); + return reject_failed_native_tool_result(slug, &outcome.result) + .map(|_| native_tool_payload(&outcome.result)); } // Autonomy-tier gate (Phase 2, made effect-aware): the node's @@ -3567,6 +3626,73 @@ mod tests { use super::*; use crate::openhuman::agent::prompts::types::IntegrationConnection; use crate::openhuman::composio::{ComposioExecuteResponse, ConnectedIntegration}; + use crate::openhuman::skills::types::{ToolContent, ToolResult}; + + // ── native `oh:` tool result handling ────────────────────────────────── + + #[test] + fn native_tool_payload_unwraps_a_single_json_block() { + // `storage_get_link` returns exactly one Json block. A downstream node + // must be able to bind `=nodes..item.json.url` — the same shape + // used everywhere else — not `...item.json.content[0].data.url`. + let result = ToolResult::json(json!({ + "url": "https://example.test/presigned", + "expires_at": "2026-01-01T00:00:00Z", + })); + let payload = native_tool_payload(&result); + assert_eq!(payload["url"], "https://example.test/presigned"); + assert_eq!(payload["expires_at"], "2026-01-01T00:00:00Z"); + assert!( + payload.get("content").is_none() && payload.get("is_error").is_none(), + "the ToolResult envelope must not leak into item.json: {payload}" + ); + } + + #[test] + fn native_tool_payload_collapses_text_to_a_bindable_field() { + let payload = native_tool_payload(&ToolResult::success("done")); + assert_eq!(payload["text"], "done"); + } + + #[test] + fn native_tool_payload_collapses_mixed_blocks_to_text() { + let result = ToolResult { + content: vec![ + ToolContent::Text { + text: "line".into(), + }, + ToolContent::Json { + data: json!({"k": 1}), + }, + ], + is_error: false, + markdown_formatted: None, + }; + let payload = native_tool_payload(&result); + let text = payload["text"].as_str().expect("text field"); + assert!(text.contains("line") && text.contains('k'), "got {text}"); + } + + #[test] + fn native_tool_failure_fails_the_step_instead_of_recording_success() { + // The bug this guards: `execute_tool` returns Ok for a tool that ran + // and FAILED (is_error), so the engine recorded the step — and the run + // — as Success while a downstream node bound a null value. + let result = ToolResult::error("storage quota exceeded"); + let err = reject_failed_native_tool_result("oh:storage_upload_file", &result) + .expect_err("an is_error ToolResult must fail the step"); + let msg = format!("{err:?}"); + assert!( + msg.contains("storage_upload_file") && msg.contains("storage quota exceeded"), + "error must name the tool and the provider detail: {msg}" + ); + } + + #[test] + fn native_tool_success_passes_through() { + let result = ToolResult::json(json!({"file_id": "f_1"})); + assert!(reject_failed_native_tool_result("oh:storage_upload_file", &result).is_ok()); + } // ── reject_unsuccessful_composio_response (B6) ────────────────────────── @@ -5660,11 +5786,7 @@ mod tests { "schema": { "type": "array" } } }); - let result = build_agent_result( - "agent-1", - "Here is the list: [1, 2, 3]", - &request, - ); + let result = build_agent_result("agent-1", "Here is the list: [1, 2, 3]", &request); assert_eq!(result, json!([1, 2, 3])); } @@ -5699,7 +5821,8 @@ mod tests { "schema": { "type": "object" } } }); - let text = "Some text\n```json\n{\"from_fence\": true}\n```\nmore text { \"from_brace\": true }"; + let text = + "Some text\n```json\n{\"from_fence\": true}\n```\nmore text { \"from_brace\": true }"; let result = build_agent_result("agent-1", text, &request); assert_eq!(result, json!({ "from_fence": true })); }