fix(flows): make native oh: tool results usable and fail loudly, document the real file-attachment chain - #5148
Merged
graycyrus merged 9 commits intoJul 24, 2026
Conversation
…cument the real attachment chain Three defects found while proving out file attachment end to end against the live Composio catalog. 1. Native tool failures were silently swallowed. The `oh:` branch of `OpenHumanTools::invoke` serialized the `ToolResult` without ever inspecting `is_error`, so a tool that ran and FAILED (quota exceeded, file missing, no integration client) recorded the step, and therefore the run, as Success. A downstream node then bound a null value and the run still reported "completed". `reject_failed_native_tool_result` mirrors the contract `reject_unsuccessful_composio_response` already provides for Composio. 2. Native tool output was unbindable in practice. Serializing the whole `ToolResult` put the envelope on `item.json`, so reaching a field required `=nodes.<id>.item.json.content[0].data.<field>`, an expression that evaluates but that no builder agent ever emits. `native_tool_payload` returns a lone `Json` block's `data` directly, so native nodes bind with the same `=nodes.<id>.item.json.<field>` shape as everything else. This also makes prompt.md's existing claim (native `oh:` tool_calls carry no `data` wrapper and use the plain form) actually true. 3. Presigned storage links leaked into the approval UI and durable storage. `redact_args` did not treat file handoff params as sensitive, so a presigned URL, which is a bearer capability until it expires, appeared verbatim on the approval card and in the persisted approval record. Prompt guidance now documents the chain that actually works: produce the file, `oh:storage_upload_file`, `oh:storage_get_link` with a short TTL, then bind the returned `url` into the send action's file parameter. The file parameter is found by its `file_uploadable: true` marker in `get_tool_contract`, not by name (Gmail calls it `attachment`, Jira calls it `file_to_upload`).
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe pull request adds native tool payload normalization and failure handling, upload-path validation, opaque upstream binding support, workflow-builder guidance for file attachments, and redaction for file-handoff URLs. It also adds regression tests and reformats provider code. ChangesWorkflow runtime and authoring
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant WorkflowBuilder
participant AuthoringGates
participant Storage
participant ProviderAction
WorkflowBuilder->>AuthoringGates: Submit file attachment workflow
AuthoringGates->>AuthoringGates: Validate path and bindings
WorkflowBuilder->>Storage: Upload workspace-relative file
Storage-->>WorkflowBuilder: Return file_id and time-limited URL
WorkflowBuilder->>ProviderAction: Pass URL through file parameter
Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Comment |
The first draft of this section leaked provider specifics into a rule whose whole point is "do not assume provider specifics": it named which toolkit calls the parameter what, and restated one provider's message size cap. That cap is already in the live contract's `description`, so repeating it in the prompt both duplicates `get_tool_contract` and undercuts the instruction to ground in it. Remembered limits also go stale silently. Now the rule stands on the `file_uploadable` marker alone, and explicitly sends the model to the contract for arity and limits. Provider names are gone from the section; the only named tools left are our own (`storage_upload_file`, `storage_get_link`), which are the actual API being described.
The first version of this chain had the producing agent call
`storage_upload_file` and emit a `file_id` through `output_parser`. That routes
the file handle through model-authored JSON, which is exactly the failure mode
that kills these runs in practice (`missing required property file_path` /
`file_id`, after the harness falls back to the `{text}` shape).
Split the upload into its own `tool_call` node and have the producer write to a
path chosen by the builder. The upload node then references that path as a
literal, so `file_id` is a node output rather than model-authored JSON, and no
`output_parser` sits anywhere in the file path. The producing agent's side
effect on disk is what matters, not its output.
Closed
5 tasks
senamakel
reviewed
Jul 23, 2026
senamakel
left a comment
senamakel
left a comment
Member
There was a problem hiding this comment.
Automated technical review: not approved.
Code correctness: confirmed. All three fixes are correctly implemented, well-tested, and follow the existing patterns in the codebase.
- reject_failed_native_tool_result (caps.rs:2631-2690) correctly mirrors
reject_unsuccessful_composio_responseand prevents silent success-on-failure for nativeoh:tools. - native_tool_payload (caps.rs:2692-2724) correctly unwraps the
ToolResultenvelope so downstream nodes bind=nodes.<id>.item.json.<field>directly, matching the documented contract inprompt.md. - SENSITIVE_KEYS additions (redact.rs:69-86) correctly add file-handoff param names. The test at redact.rs:456-478 verifies presigned URLs are redacted from both approval display and durable storage.
- prompt.md guidance accurately documents the four-step file-attachment chain discovered against the live Composio catalog.
- Tests: 5 new case in caps.rs (single-Json unwrap, text collapse, mixed-blocks collapse, error rejection, success pass-through) plus 1 in redact.rs (presigned-link leak). All follow existing test conventions.
Unmet gate:
- CI is still in progress: 2 checks pending (Rust Core Coverage, Rust RSS Benchmark). 0 failing, 12 passing/skipped. Pending CI is a bright-line gate per review policy.
Action: Once the pending checks complete and confirm green (expected, since Rust Feature-Gate Smoke and Rust Quality already passed on the same code), the PR is ready for approval. No code issues to address.
…h: upstream The null-arg author-gate (`validate_required_arg_resolvability`) and the dry-run `null_resolutions` diagnostic both delegate "is this null unverifiable vs a real wiring bug?" to one helper, which excluded native `oh:` tool_call upstreams. So a Composio send node binding `=nodes.get_link.item.json.url` from a native `oh:storage_get_link` node fell through to a HARD REJECT, because that url is null in the echo sandbox (native tools are opaque-echoed, exactly like Composio ones). This blocked the exact produce -> upload -> get_link -> send chain the attachment guidance prescribes: an independent judge traced a live "fix with agent" self-repair loop that built the correct chain, got gate-rejected four times, and halted on repeated tool failure. Native and Composio tool_call outputs are both opaque in the sandbox, so a null bound to either is unverifiable, not broken. Generalize `composio_tool_call_upstream_ref` -> `mock_opaque_tool_call_upstream_ref` and drop the `oh:` exclusion (keep excluding `=`-dynamic slugs). The dry-run suggestion now adapts to the upstream kind: a native `oh:` upstream binds FLAT at `.item.json.<field>` (no `.data.` wrapper, no Composio `get_tool_contract`), so pointing it at the Composio `.data.` advice would send the agent chasing a path that never exists. The gate is NOT weakened: an agent/code/transform/trigger upstream null still hard-rejects (B18 intact, guarded by the unchanged reject test). Adds a direct unit for the helper, a downgrade test for the native-upstream case, and the drift check that was missing pre-merge (author the documented native chain, assert the gate passes it).
Two bugs the judge found in the attachment section's worked example: - It wrote the file to an absolute `/tmp/openhuman-flow/report.html`, which is dead on arrival: `resolve_upload_path` confines uploads to the agent workspace and the file_write policy blocks writes outside it. Use a workspace-relative path (`report.html`), and say plainly that an absolute path outside the workspace is rejected. - It recommended `expires_in_seconds: 300`, but the outbound send is parked for human approval for up to ~10 minutes; a user who approves late hands the provider a dead URL. Raise the recommended TTL to 900 with a note that it must outlive the approval window (and not run far longer than needed, since the URL is a bearer capability while it lives).
…ol-results-and-file-attachment
The merge brought pre-existing formatting drift on `main` (provider.rs, caps.rs) that the CI toolchain (1.96.1) flags but a newer local rustfmt did not. Reformatted with the pinned toolchain so `cargo fmt --all -- --check` passes. No logic change.
…author time Live testing showed the builder proposing an `oh:storage_upload_file` node with `path: /tmp/openhuman-flow/report.html` — an absolute path the runtime `resolve_upload_path` rejects (uploads are confined to the agent workspace), so the run dies at the upload step. The standing prompt already says to use a workspace-relative path, and the running binary carries that guidance, but the model ignores it and copies an absolute path from a prior flow's example. This is the recurring "prompt guidance does not reliably steer the builder" failure, so enforce it in code the same way the gate carve-out did. Adds a cheap sync author-gate `validate_upload_paths`: an `oh:storage_upload_file` node whose LITERAL `path` is absolute (`/tmp/...`, `/Users/...`) or escapes with `..` is rejected with an actionable message naming a relative example. A `=`-expression path is left to the runtime (its value is unknown at author time); an absent path is left to the required-arg gate. Wired into `run_builder_gates`. Scope note: this enforces the UPLOAD side. The producing node's write path lives in that agent's natural-language prompt and cannot be structurally gated; a mismatch there still surfaces loudly (file_write policy + upload "file not found") rather than silently.
4 tasks
graycyrus
marked this pull request as ready for review
July 24, 2026 12:55
coderabbitai
Bot
added
bug
rust-core
|
| Filename | Overview |
|---|---|
| src/openhuman/tinyflows/caps.rs | Adds reject_failed_native_tool_result (mirrors Composio branch) and native_tool_payload (unwraps single-Json-block ToolResult to flat item.json), wiring both into the oh: invoke path. Logic is correct and thoroughly unit-tested. |
| src/openhuman/flows/ops.rs | Renames composio_tool_call_upstream_ref → mock_opaque_tool_call_upstream_ref and removes the oh: exclusion so native tool_call nulls are treated as unverifiable (not genuine errors). Adds validate_upload_paths gate for workspace-relative path enforcement. |
| src/openhuman/approval/redact.rs | Adds file-handoff keys to SENSITIVE_KEYS; intentionally omits generic url/link. Test explicitly asserts bare url stays visible on approval card. |
| src/openhuman/flows/builder_tools.rs | Emits different disambiguation advice for native oh: upstreams vs Composio upstreams so the agent is never sent chasing a non-existent .data. wrapper. |
| src/openhuman/flows/ops_tests.rs | Adds 7 new tests covering mock_opaque_tool_call_upstream_ref, native-upstream resolvability downgrade, full attachment chain, and validate_upload_paths. |
| src/openhuman/agent/harness/subagent_runner/ops/provider.rs | Formatting-only (cargo fmt). No logic changes. |
| src/openhuman/flows/agents/workflow_builder/prompt.md | Adds four-node produce→upload→get_link→send chain documentation with explicit traps. |
Sequence Diagram
sequenceDiagram
participant P as produce node
participant U as oh:storage_upload_file
participant L as oh:storage_get_link
participant S as Composio send action
participant C as Composio backend
P->>U: writes file to workspace-relative path
Note over U: validate_upload_paths gate rejects absolute/.. paths
U->>U: reject_failed_native_tool_result
U->>U: native_tool_payload → flat item.json.file_id
U-->>L: "=nodes.upload.item.json.file_id"
L->>L: reject_failed_native_tool_result
L->>L: native_tool_payload → flat item.json.url
L-->>S: "=nodes.get_link.item.json.url"
Note over S: approval card: attachment key redacted
S->>C: executes with attachment URL
C-->>S: attachment landed
Reviews (2): Last reviewed commit: "fix(approval): don't over-redact bare ur..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 056bb4d367
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // 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('=') { |
There was a problem hiding this comment.
.data bindings instead of downgrading
When a downstream Composio action binds to a native oh: tool using the old Composio-shaped path, e.g. =nodes.get_link.item.json.data.url, this helper now returns Some solely because the upstream slug is non-dynamic; validate_required_arg_resolvability then treats the null as unverifiable and skips the hard rejection. But native_tool_payload unwraps native results flat at .item.json.<field>, so .json.data.* can never exist for oh:storage_get_link; an attachment flow can pass author gates and later send without the attachment or fail at runtime. Please only downgrade the flat native shape, or explicitly reject .json.data on native upstreams.
Useful? React with 👍 / 👎.
Contributor
Author
There was a problem hiding this comment.
Valid edge case, but I'm leaving the carve-out as-is rather than rejecting .data. on native upstreams:
- False positives.
native_tool_payloadunwraps a singleJsonblock to itsdatavalue directly, so a native tool whose payload itself has a top-leveldatafield legitimately exposes.item.json.data.<field>. Blanket-rejecting.item.json.data.*on native upstreams would re-introduce the exact spurious-rejection class this carve-out removes, and the gate can't distinguish a wrong.data.from a valid one without the tool's real output contract (not available here). - The sandbox can't verify either shape — both
.item.json.urland.item.json.data.urlresolve null in the echo sandbox, which is precisely why they're downgraded tounverifiablerather than asserted.
And it doesn't fail silently: a wrong .data. path on a flat native tool resolves null at runtime, the send gets an empty attachment, and the provider rejects it loudly via reject_failed_native_tool_result / reject_unsuccessful_composio_response (verified live: an empty attachment yields Either path or blob must be provided). Happy to revisit with a contract-aware check as a follow-up if it proves a real problem.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/openhuman/approval/redact.rs`:
- Around line 72-89: Remove the generic "url" and "link" entries from
SENSITIVE_KEYS so approval-visible destination and argument fields remain
readable, while retaining the specific file-link keys. Update
file_handoff_links_are_redacted to remove its bare-"url" assertion, and verify
that the oh:storage_get_link attachment-binding output uses one of the remaining
specific keys such as attachment, file_to_upload, or public_url.
In `@src/openhuman/flows/agents/workflow_builder/prompt.md`:
- Around line 588-590: Update the wording in the prompt text to hyphenate
“world-readable” when describing the object’s accessibility, without changing
the surrounding guidance.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1778bbf0-78af-4606-a443-80fa49b4c62a
📒 Files selected for processing (7)
src/openhuman/agent/harness/subagent_runner/ops/provider.rssrc/openhuman/approval/redact.rssrc/openhuman/flows/agents/workflow_builder/prompt.mdsrc/openhuman/flows/builder_tools.rssrc/openhuman/flows/ops.rssrc/openhuman/flows/ops_tests.rssrc/openhuman/tinyflows/caps.rs
CodeRabbit + Greptile both flagged that adding bare `"url"` and `"link"` to `SENSITIVE_KEYS` over-redacts: `redact_args` is shared across native, Composio, and `http_request` approval paths, so it would blank out an `http_request` node's destination URL and any Composio action's benign `url`/`link` arg on the approval card — hiding exactly the information a human approver needs to judge an outbound action. The presigned-link threat is already covered by the specific file-handoff keys (`attachment`, `file_to_upload`, `file_url`, `public_url`, `signed_url`, `presigned_url`) — the link binds into a `file_uploadable` parameter like `attachment`, never a key literally named `url`. Drops `"url"` and `"link"`; the redaction test now also asserts a bare `url` (an http_request destination) stays VISIBLE. Also hyphenates "world-readable" in the prompt guidance (CodeRabbit nit).
coderabbitai
Bot
removed
the
rust-core
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Replaces the closed #5122 and #5134, which were built on a slug that does not exist. This PR fixes the three defects that were actually blocking file attachments in flows, verified against the live Composio catalog and a real send.
What we proved first (no code was written before this)
Queried the live contract via
openhuman.flows_get_tool_contract:Then executed a real
GMAIL_SEND_EMAILwithattachmentset to a public HTTPS URL, and fetched the sent message back: 1 real attachment landed. So Composio dereferences an HTTPS URL in afile_uploadableparam.Conclusions:
GMAIL_SEND_EMAIL_WITH_ATTACHMENTaction.GMAIL_SEND_EMAILtakes the attachment directly.file_uploadable: truemarker, which is provider agnostic. Gmail names itattachment;JIRA_ADD_ATTACHMENTnames itfile_to_upload.Error reading file at /Users/... ENOENT.The three fixes
1. Native
oh:tool failures were recorded as Success. Theoh:branch ofOpenHumanTools::invokeserialized theToolResultwithout inspectingis_error, so a tool that ran and failed (quota exceeded, file missing, no integration client) still marked the step, and the run,Success. A downstream node then boundnulland the run reported "completed".reject_failed_native_tool_resultmirrors the guardreject_unsuccessful_composio_responsealready gives the Composio branch.2. Native tool output was unbindable in practice. Serializing the whole
ToolResultput the envelope onitem.json, so reaching a field required=nodes.<id>.item.json.content[0].data.<field>. That evaluates, but no builder agent ever emits it.native_tool_payloadreturns a loneJsonblock'sdatadirectly, so native nodes bind with the same=nodes.<id>.item.json.<field>shape as everything else.3. Presigned storage links leaked into the approval UI and durable storage.
redact_argsdid not treat file-handoff params as sensitive, so a presigned URL, which is a bearer capability until it expires, appeared verbatim on the approval card and in the persisted approval record. Addedattachment,attachments,file_to_upload,file_url,url,link,public_url,signed_url,presigned_urltoSENSITIVE_KEYS.Prompt guidance
Documents the chain that actually works, and the traps that cost two PRs:
oh:storage_upload_file.oh:storage_get_linkwith a shortexpires_in_seconds(300).urlinto the send action's file parameter.Explicitly: find the file param by its
file_uploadablemarker rather than guessing a name; never upload withvisibility: "public"(permanently world readable, whereas the presigned link expires); never bind a local path; body content does not satisfy an attachment request; and aget_tool_contract"not a real action" result is a hard stop.Testing
is_errorrejection, and the success pass-through.cargo test --lib -- openhuman::flows openhuman::tinyflows openhuman::approval: 701 passed, 0 failed.cargo checkclean,cargo fmtapplied, vendor submodule pointers verified unchanged vsupstream/main.Known follow-ups (not in this PR)
main:LINEAR_CREATE_ATTACHMENTandAIRTABLE_UPLOAD_ATTACHMENTare curated today and both return "not a real action in the live catalog".flow_tool_allowedPath A treats curated-list membership as the only existence check, while Path B verifies liveness. A CI job validating curated catalogs against the live catalog would have caught feat(flows): enable agent file output → attachment via the storage-URL path (B39 phase 1-2) #5122's fake slug and catches these.storage_get_linkURL uses the same mechanism but its host must be reachable from Composio's servers. Worth one live confirmation.output_parserschemas hard-fail runs when the model's output does not match exact required field names. That killed more real runs than the attachment issue.Blast radius
item.jsonfor existingoh:tool_callnodes from theToolResultenvelope to the payload. Any saved flow bindingcontent[0].data.*would break; the native path is new enough that this is likely zero, but worth a look.is_errorfix will turn some previously "successful" runs red. That is correct: they were silently no-ops.Submission Checklist
cargo fmt/ formatting appliedSummary by CodeRabbit
New Features
Bug Fixes
Documentation