mirror of
https://github.com/tinyhumansai/openhuman.git
synced 2026-07-27 21:08:00 +00:00
fix(flows): plain-language workflow_builder replies + read-only memory (#4869)
This commit is contained in:
@@ -1017,7 +1017,12 @@ mod tests {
|
||||
// (hard-refused otherwise, regardless of the user's scope preference)
|
||||
// against an already-connected toolkit — see `builder_tools.rs`'s
|
||||
// module doc. This pins the invariant in the agent definition itself,
|
||||
// not just the tool implementations.
|
||||
// not just the tool implementations. It also has read-only grounding
|
||||
// in the user's memory via `memory_recall` (direct lookups) and
|
||||
// `memory_hybrid_search` (keyword/lexical lookups — pairs with
|
||||
// `memory_recall` the same way the sibling `flow_discovery` agent
|
||||
// does) — no `memory_store`, so it can look up context but never
|
||||
// write it.
|
||||
let def = find("workflow_builder");
|
||||
assert_eq!(def.agent_tier, AgentTier::Worker);
|
||||
assert_eq!(def.delegate_name.as_deref(), Some("build_workflow"));
|
||||
@@ -1051,6 +1056,8 @@ mod tests {
|
||||
"composio_list_toolkits",
|
||||
"composio_list_connections",
|
||||
"composio_connect",
|
||||
"memory_recall",
|
||||
"memory_hybrid_search",
|
||||
];
|
||||
for required in expected {
|
||||
assert!(
|
||||
@@ -1078,6 +1085,8 @@ mod tests {
|
||||
"apply_patch",
|
||||
"composio_execute",
|
||||
"spawn_subagent",
|
||||
// Memory access must stay read-only: no write tool.
|
||||
"memory_store",
|
||||
] {
|
||||
assert!(
|
||||
!names.iter().any(|n| n == forbidden),
|
||||
|
||||
@@ -46,6 +46,25 @@ hint = "reasoning"
|
||||
# listing publishes no output schema at all (verified for every GitHub
|
||||
# action), so a downstream split_out.path is never guessed. See
|
||||
# `builder_tools.rs`'s module doc.
|
||||
# `memory_recall` is the raw (un-delegated) memory read tool — READ-ONLY, no
|
||||
# write counterpart (`memory_store` is deliberately absent). This agent is a
|
||||
# Named-scope agent, so the orchestrator's `retrieve_memory` delegate would
|
||||
# not synthesize context for it; the raw tool is the correct mechanism here.
|
||||
# `omit_memory_context = true` stays set above, so nothing is injected
|
||||
# automatically — the agent must call `memory_recall` explicitly when it
|
||||
# needs the user's context.
|
||||
# `memory_hybrid_search` is paired alongside it (same convention as the
|
||||
# sibling `flow_discovery` agent) for keyword/lexical lookups — the
|
||||
# `[learning]`-gated `MemoryAccessSection` bias instruction
|
||||
# (`src/openhuman/learning/prompt_sections.rs`) tells every agent with a
|
||||
# retrieval tool visible to "call `memory_recall` (or `memory_search` for
|
||||
# keyword lookups)"; `memory_search` itself is not a registered tool name
|
||||
# anywhere in this codebase (a pre-existing, wider naming inconsistency
|
||||
# across several agent configs, out of scope here), so without a real
|
||||
# keyword-capable tool visible, a keyword-memory turn on this Named scope
|
||||
# could only ever reach `memory_recall` — leaving the guidance's "keyword
|
||||
# lookups" half unsatisfiable. `memory_hybrid_search`'s `lexical` mode is
|
||||
# the real keyword-heavy retrieval this agent needs; both stay READ-ONLY.
|
||||
named = [
|
||||
"propose_workflow",
|
||||
"revise_workflow",
|
||||
@@ -63,4 +82,6 @@ named = [
|
||||
"composio_list_toolkits",
|
||||
"composio_list_connections",
|
||||
"composio_connect",
|
||||
"memory_recall",
|
||||
"memory_hybrid_search",
|
||||
]
|
||||
|
||||
@@ -293,6 +293,50 @@ mod tests {
|
||||
assert!(p.contains("END-TO-END"));
|
||||
}
|
||||
|
||||
/// The standing archetype (`prompt.md`, the always-loaded system prompt —
|
||||
/// as opposed to the per-turn directives rendered above) carries the same
|
||||
/// B27 banned-phrase regression, plus positive coverage for the plain-
|
||||
/// language style rule and the read-only memory grounding tool added
|
||||
/// alongside it. Guards against reintroducing jargon-leaking or
|
||||
/// phantom-review-card language, and against silently losing the
|
||||
/// `memory_recall` guidance if the prompt is ever rewritten.
|
||||
#[test]
|
||||
fn standing_prompt_teaches_plain_language_and_readonly_memory() {
|
||||
const STANDING_PROMPT: &str = include_str!("prompt.md");
|
||||
|
||||
// Negative (B27): the phantom "review card" phrasing must never
|
||||
// reappear in the standing prompt either.
|
||||
for banned in ["review card", "Accept the proposal explicitly"] {
|
||||
assert!(
|
||||
!STANDING_PROMPT.contains(banned),
|
||||
"standing prompt must not carry phantom review-card phrasing `{banned}` (B27)"
|
||||
);
|
||||
}
|
||||
|
||||
// Positive: the anti-jargon Style rule — replies must stay in plain
|
||||
// language, never leak response_format/schema/expression internals.
|
||||
assert!(
|
||||
STANDING_PROMPT.contains("Speak to a non-technical user"),
|
||||
"standing prompt must teach the anti-jargon Style rule"
|
||||
);
|
||||
|
||||
// Positive: read-only memory grounding via the raw `memory_recall`
|
||||
// tool (no `memory_store` — see the agent.toml regression test).
|
||||
assert!(
|
||||
STANDING_PROMPT.contains("memory_recall"),
|
||||
"standing prompt must teach the builder to ground itself with memory_recall"
|
||||
);
|
||||
|
||||
// Positive: the prompt must state the read-only contract explicitly —
|
||||
// not just mention the tool name — so a future edit can't silently
|
||||
// drop the "can't change their memory" guarantee this agent's tool
|
||||
// scope depends on (no `memory_store` in agent.toml).
|
||||
assert!(
|
||||
STANDING_PROMPT.contains("Read-only — you can't change their memory"),
|
||||
"standing prompt must state the memory read-only guarantee, not just mention memory_recall"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn repair_includes_run_id_error_and_failing_nodes() {
|
||||
let mut r = req(BuildMode::Repair);
|
||||
|
||||
@@ -19,45 +19,37 @@ no tool that does — by design. Your authoring outputs are:
|
||||
- **`save_workflow`** — the ONE persistence tool you have, and it only writes to
|
||||
a flow that **already exists** (you need its `flow_id`). See below.
|
||||
|
||||
If there is no existing flow to save to, only the user's own explicit click
|
||||
persists it — never yours, and the click looks different depending on where
|
||||
your proposal shows up: on the canvas copilot, Accept applies it to the
|
||||
draft and the canvas's own **Save** persists it; in main chat or Suggested
|
||||
Workflows (no flow open yet), your proposal renders as a card whose
|
||||
**"Save & enable"** button calls the save RPC directly. Either way, saving
|
||||
is the user's action, not a tool you have. If a user says "just turn it on
|
||||
for me", explain that enabling stays in their hands — you cannot enable a
|
||||
flow.
|
||||
Persisting is otherwise the user's own action, not a tool you have — the one
|
||||
exception is `save_workflow` on an **existing** flow id, and only when the
|
||||
user **explicitly asks** (see below). If a user says "just turn it on for
|
||||
me", explain that enabling stays in their hands — you cannot enable a flow.
|
||||
|
||||
## Saving your work: `save_workflow` (only on the user's explicit ask)
|
||||
|
||||
Every authoring turn — including a **build** turn seeded from the Flows
|
||||
prompt bar (which creates the flow first and delegates with its id) and every
|
||||
**revise** turn on the canvas copilot — is **propose-only** by default. Your
|
||||
arc is:
|
||||
Every authoring turn — build, revise, or repair — is **propose-only** by
|
||||
default. Your arc is:
|
||||
|
||||
1. Ground + build the graph (below), `dry_run_workflow` until it's clean.
|
||||
2. `revise_workflow` / `propose_workflow` so the user sees the proposal.
|
||||
**Stop there** and hand back — the user reviews and persists it
|
||||
themselves. On the canvas copilot: Accept applies it to the draft, then
|
||||
the canvas's own Save persists it. In main chat or Suggested Workflows
|
||||
(no flow open yet): the proposal renders as a card and "Save & enable"
|
||||
persists it directly. Do NOT call `save_workflow` unless the user
|
||||
explicitly asks.
|
||||
2. `propose_workflow` / `revise_workflow` so the user sees the proposal, then
|
||||
**stop and hand back** — persisting it is their action, not yours. Don't
|
||||
over-explain how to save: give one short line for the current surface
|
||||
("accept it on the canvas and hit Save", or "use Save & enable on the
|
||||
card") — never recite every persist path, and never repeat it across
|
||||
turns.
|
||||
|
||||
**Do NOT auto-`save_workflow` when the request carries a `flow_id`.** The id
|
||||
is context — the user may later ask you to save/test that flow — but the
|
||||
persistence gate stays with the user. Auto-saving would leave the flow's
|
||||
graph persisted even if the user Rejects the proposal.
|
||||
**When the user says "save it":** if you have a `save_workflow` action
|
||||
available — an **existing** `flow_id` plus their explicit ask ("save this",
|
||||
"yes save it onto flow_X") — just call `save_workflow { flow_id, graph,
|
||||
name? }` and confirm in one plain line what you saved (trigger, steps, and —
|
||||
if the flow is enabled with a schedule/app_event trigger — that it's now
|
||||
live and will fire on its own). If you don't have that (no flow yet, or they
|
||||
haven't asked), give the one short line above instead of re-explaining.
|
||||
|
||||
Use **`save_workflow { flow_id, graph, name? }`** only when the user
|
||||
**explicitly asks** you to save it ("save this", "yes save it onto flow_X").
|
||||
When you do, tell them plainly what you saved (trigger, steps, and — if the
|
||||
flow is enabled with a schedule/app_event trigger — that it is now live and
|
||||
will fire on its own). Never `save_workflow` onto a flow the user did NOT
|
||||
ask you to build/update — editing some other saved flow requires their
|
||||
explicit ask naming it. It cannot create flows, and it never changes
|
||||
`enabled` or the approval gate.
|
||||
**Do NOT auto-`save_workflow`** just because the request carries a
|
||||
`flow_id` — the id is context for a later ask, but the persistence gate
|
||||
stays with the user until they explicitly ask. Never `save_workflow` onto a
|
||||
flow the user did NOT ask you to build/update. It cannot create flows, and
|
||||
it never changes `enabled` or the approval gate.
|
||||
|
||||
## Testing a saved flow: `run_flow` (ask first!)
|
||||
|
||||
@@ -78,6 +70,16 @@ it as real). Rules:
|
||||
report what happened; if it failed, `get_flow_run` for the steps and propose a
|
||||
fix.
|
||||
|
||||
## Grounding in what you already know: `memory_recall`
|
||||
|
||||
You can `memory_recall` to look up the user's context — connected channels,
|
||||
teammates/people, stated preferences, past decisions. Use it to resolve a
|
||||
genuinely-ambiguous target/recipient/preference **before** asking or
|
||||
guessing (e.g. recall their default channel or their team's names). For a
|
||||
keyword-style lookup (a specific name, term, or phrase you need to find
|
||||
rather than a general context recall), use `memory_hybrid_search` in its
|
||||
`lexical` mode instead. Read-only — you can't change their memory.
|
||||
|
||||
## Your authoring loop
|
||||
|
||||
1. **Understand the trigger and the steps.** What starts the flow? What should
|
||||
@@ -229,6 +231,12 @@ A `WorkflowGraph` is `{ name?, nodes: [...], edges: [...] }`.
|
||||
the `true` branch instead. Typing the field as `boolean` in the schema is
|
||||
what makes the output-parser coerce/validate it into a real boolean rather
|
||||
than a string that merely looks like one.
|
||||
|
||||
An `agent` node inside a workflow can also **read and write the user's
|
||||
memory at run time**. If a workflow genuinely needs the user's context
|
||||
(recall a preference) or should remember a result/state across runs, wire
|
||||
an `agent` node that uses memory instead of hardcoding context memory
|
||||
already holds. Use sparingly — only when the workflow truly needs it.
|
||||
3. **`tool_call`** — an action. Two flavours by `config.slug`:
|
||||
- **Composio app action** — `config.slug` = a real action slug (from
|
||||
`search_tool_catalog`, e.g. `GMAIL_SEND_EMAIL`) + `config.connection_ref`
|
||||
@@ -486,6 +494,15 @@ failure at runtime. Rules of thumb:
|
||||
|
||||
## Style
|
||||
|
||||
**Speak to a non-technical user.** Describe what the workflow *does* in plain
|
||||
language; never surface implementation internals in your replies — no
|
||||
`response_format`, `output_parser.schema`, jq/`=`-expressions, node config
|
||||
JSON, tool slugs, or envelope-path talk — unless the user explicitly asks how
|
||||
it's wired. Say "it'll read your unread email and post a summary to
|
||||
`#team-product` every morning", not "I added an agent node with an
|
||||
output_parser.schema and bound the Slack node to
|
||||
=nodes.research.item.json…".
|
||||
|
||||
Be concise. Your posture is **clarify genuinely-ambiguous inputs, verify before
|
||||
you propose, and don't stop until the graph is right** — but a workflow that
|
||||
needs zero questions is still the happy path. Don't let "ask when truly
|
||||
@@ -550,8 +567,10 @@ values appear:
|
||||
PLACEHOLDER (e.g. `""`) rather than null, so the dry run reports
|
||||
`ok: true`. This is expected — the schema is correctly declared, the
|
||||
binding path is correct, and at runtime a real LLM will produce real
|
||||
values. You may note this: "the dry run passed; the agent node's output
|
||||
is a mock placeholder — at runtime the real model fills these in."
|
||||
values. Treat this as your own internal confirmation that the wiring is
|
||||
correct; don't narrate the mock/placeholder mechanics to the user — that's
|
||||
sandbox internals, not something they need to hear. Just tell them,
|
||||
plainly, that the workflow checks out.
|
||||
|
||||
2. **Real binding nulls** — a `=nodes.<id>.item.json.<field>` expression
|
||||
that resolves to `null` because the path is WRONG (missing `.json.`,
|
||||
|
||||
@@ -2170,13 +2170,20 @@ async fn validate_tool_contracts_skips_rather_than_rejects_when_the_catalog_is_u
|
||||
// `missing_required_args` because SOME value was present, just under the
|
||||
// wrong key) ────────────────────────────────────────────────────────────
|
||||
|
||||
/// `SLACK_SEND_MESSAGE` with a real `input_schema` naming `channel` and
|
||||
/// `markdown_text` — models the live bug this fixes: `markdown_text` is the
|
||||
/// real field, `text` is not.
|
||||
/// Models `SLACK_SEND_MESSAGE`'s real `input_schema` (naming `channel` and
|
||||
/// `markdown_text` — the live bug this fixes: `markdown_text` is the real
|
||||
/// field, `text` is not) but under a **fictional toolkit key**
|
||||
/// (`slackargnametest`), never the real `"slack"` key: `seeded_slack_send_contract`
|
||||
/// above (input_schema: `None`) also seeds `"slack"` and is used by several
|
||||
/// sibling tests in this file whose `args` still carry `text` — sharing the
|
||||
/// real key would race those tests over the process-global
|
||||
/// `LIVE_CATALOG_CACHE` entry for `"slack"` (same discipline
|
||||
/// `builder_tools_tests.rs` already applies for its own `slack`/`gmail`
|
||||
/// fixtures that don't match the shared-key contract byte-for-byte).
|
||||
fn seeded_slack_send_message_contract_with_schema() -> ToolContract {
|
||||
ToolContract {
|
||||
slug: "SLACK_SEND_MESSAGE".to_string(),
|
||||
toolkit: "slack".to_string(),
|
||||
slug: "SLACKARGNAMETEST_SEND_MESSAGE".to_string(),
|
||||
toolkit: "slackargnametest".to_string(),
|
||||
description: None,
|
||||
required_args: vec![],
|
||||
input_schema: Some(json!({
|
||||
@@ -2196,7 +2203,7 @@ fn seeded_slack_send_message_contract_with_schema() -> ToolContract {
|
||||
#[tokio::test]
|
||||
async fn validate_tool_contracts_rejects_an_arg_name_not_in_the_input_schema() {
|
||||
seed_live_catalog_cache(
|
||||
"slack",
|
||||
"slackargnametest",
|
||||
vec![seeded_slack_send_message_contract_with_schema()],
|
||||
);
|
||||
let config = Config::default();
|
||||
@@ -2204,7 +2211,7 @@ async fn validate_tool_contracts_rejects_an_arg_name_not_in_the_input_schema() {
|
||||
"nodes": [
|
||||
{ "id": "t", "kind": "trigger", "name": "Manual" },
|
||||
{ "id": "post", "kind": "tool_call", "name": "Post",
|
||||
"config": { "slug": "SLACK_SEND_MESSAGE",
|
||||
"config": { "slug": "SLACKARGNAMETEST_SEND_MESSAGE",
|
||||
"args": { "channel": "#general", "text": "hi" } } }
|
||||
],
|
||||
"edges": [ { "from_node": "t", "to_node": "post" } ]
|
||||
@@ -2220,7 +2227,7 @@ async fn validate_tool_contracts_rejects_an_arg_name_not_in_the_input_schema() {
|
||||
#[tokio::test]
|
||||
async fn validate_tool_contracts_passes_the_real_arg_name_from_the_input_schema() {
|
||||
seed_live_catalog_cache(
|
||||
"slack",
|
||||
"slackargnametest",
|
||||
vec![seeded_slack_send_message_contract_with_schema()],
|
||||
);
|
||||
let config = Config::default();
|
||||
@@ -2228,7 +2235,7 @@ async fn validate_tool_contracts_passes_the_real_arg_name_from_the_input_schema(
|
||||
"nodes": [
|
||||
{ "id": "t", "kind": "trigger", "name": "Manual" },
|
||||
{ "id": "post", "kind": "tool_call", "name": "Post",
|
||||
"config": { "slug": "SLACK_SEND_MESSAGE",
|
||||
"config": { "slug": "SLACKARGNAMETEST_SEND_MESSAGE",
|
||||
"args": { "channel": "#general", "markdown_text": "hi" } } }
|
||||
],
|
||||
"edges": [ { "from_node": "t", "to_node": "post" } ]
|
||||
|
||||
Reference in New Issue
Block a user