From 9bd1ac03d9061ed701400e1e2ec8100ee89aeeef Mon Sep 17 00:00:00 2001 From: Eldon Marks Date: Fri, 7 Aug 2026 15:46:40 -0400 Subject: [PATCH] docs(orchestrator): pin and document channel_overrides REPLACE semantics Two properties of channel_overrides account for most "channel overrides don't work" reports. Both are by design and neither announces itself. List knobs REPLACE rather than merge. pinned_tools, denied_tools and skill_only_tools inside a channel block stand in for the action-level list on that channel, so anything still wanted must be repeated there. That is why a config which works with the block commented out can stop working once it is uncommented: the block is not adding to the global list, it is standing in for it. An explicit [] means "none here", not "fall back". Keys match visitor.channel exactly. No prefixes, no aliases, so a block written for whatsapp does not cover whatsapp_call -- voice is its own channel string and both ship in the example app. A mis-keyed block no-ops with nothing logged. Document both in ORCHESTRATOR.md under Configuration, with a table mapping each assembly warning to the failure it indicates, and state them at the two places in the example agent.yaml an operator actually reads -- the channel_overrides block and skill_only_tools. The example previously showed pinned_tools inside a channel block without noting that it replaces. Add tests/wire/test_channel_overrides_replace.py. The orchestrator suites set ex.channel_overrides in Python, which pins the resolver but never exercises agent.yaml -> bootstrap -> persisted attribute -> resolution; a loader that dropped or reshaped the nested block would leave them all green while the feature did nothing. These boot a real graph and read the orchestrator back out of the database. Mutation-checked per the wire tier's convention, which caught a hole in the first draft: the exact-match test asserted on whatsapp_call, which has its own block, so the exact lookup succeeded and a prefix-matching mutation never reached the fallback path -- the test passed while proving nothing. It now asserts on whatsapp_media, a prefix-sharing channel with no block of its own, and fails under that mutation. --- docs/ORCHESTRATOR.md | 53 +++++ .../jvagent/orchestrator_agent/agent.yaml | 23 +- tests/wire/test_channel_overrides_replace.py | 204 ++++++++++++++++++ 3 files changed, 278 insertions(+), 2 deletions(-) create mode 100644 tests/wire/test_channel_overrides_replace.py diff --git a/docs/ORCHESTRATOR.md b/docs/ORCHESTRATOR.md index cf7d50fc..95b121e0 100644 --- a/docs/ORCHESTRATOR.md +++ b/docs/ORCHESTRATOR.md @@ -317,6 +317,59 @@ actions: Pair `web_search` with `web_fetch`: search surfaces URLs, then `web_fetch__fetch` reads the top sources as clean markdown — far more efficient (and better grounded) than re-searching snippets. `web_fetch` is SSRF-guarded by default (blocks loopback/private/link-local hosts) and frames fetched text as untrusted so it composes with the loop's anti-injection boundaries. +### Per-channel overrides (`channel_overrides`) + +A voice call and a web chat can run one agent with different loop knobs — +`channel_overrides` keys a block of overrides by `visitor.channel`: + +```yaml + skill_only_tools: ["pay__*"] # applies wherever no override exists + pinned_tools: ["kb__search"] + + channel_overrides: + whatsapp_call: + history_limit: 6 # additive knobs: just set them + max_duration_seconds: 30 + skill_only_tools: # LIST knobs REPLACE — see below + - "pay__*" # repeat what you still want gated + - "wa__*" +``` + +Two properties account for most "channel overrides don't work" reports. Both are +by design, and neither announces itself at runtime. + +**1. List knobs REPLACE, they do not merge.** `pinned_tools`, `denied_tools` and +`skill_only_tools` in a channel block *replace* the action-level list on that +channel. Anything you still want must be repeated inside the block. Omit it and +it is silently absent there — which is why a config that works with +`channel_overrides` commented out can stop working when it is uncommented: the +block is not adding to the global list, it is standing in for it. An explicit +`[]` means "none here", not "fall back". Scalar knobs (`history_limit`, +`tool_call_timeout`, …) simply take the channel value. + +**2. Keys match `visitor.channel` exactly.** There is no prefix matching and no +aliasing, so a block written for `whatsapp` does **not** cover `whatsapp_call` — +voice is its own channel string, and both are valid. A mis-keyed block no-ops +and the action-level value applies, with nothing logged. When a turn behaves as +though the override were absent, confirm the channel string of *that* turn +first. + +For `skill_only_tools` specifically, a third failure is possible and does log: +gating a tool that no *reachable* skill declares makes it uncallable (fail +closed). Skills can themselves be channel-gated, so a skill that owns a gated +tool on web but is not offered on voice leaves that tool gated-and-ownerless +there. The assembly warnings distinguish the cases: + +| Log line | Meaning | +|---|---| +| `skill_only_tools patterns matched no tool` | globs are dead — **nothing is gated** | +| `matched tools no available skill declares` | gated but ownerless — **uncallable this turn** | +| *(silent)* | override key never matched the channel | + +Covered by `tests/wire/test_channel_overrides_replace.py`, which asserts the +resolution against an orchestrator read back out of a real graph rather than one +constructed in memory. + ### Model gearing (ADR-0016 / ADR-0041) Optional: pair a **light** completion model with the **heavy** reasoning model so single-dimensional turns don't pay the reasoning tax. The existing `model*`/`reasoning_*` are the heavy profile; set `light_model` (+ `light_model_action_type`, `light_model_temperature`, `light_model_max_tokens`) to engage gearing — empty leaves the agent single-model. diff --git a/examples/jvagent_app/agents/jvagent/orchestrator_agent/agent.yaml b/examples/jvagent_app/agents/jvagent/orchestrator_agent/agent.yaml index 7de5b51b..983e282a 100644 --- a/examples/jvagent_app/agents/jvagent/orchestrator_agent/agent.yaml +++ b/examples/jvagent_app/agents/jvagent/orchestrator_agent/agent.yaml @@ -145,8 +145,11 @@ actions: # uncallable (fail closed). denied_tools wins; a pinned_tools match cannot # un-gate. Empty here — this example gates nothing; set e.g. # ["payments__*"] to confine a sensitive capability to its owning skill. - # Channel-overridable via channel_overrides.skill_only_tools (REPLACES - # this action-level list on that channel). + # Channel-overridable via channel_overrides.skill_only_tools, which + # REPLACES this action-level list on that channel rather than adding to + # it — repeat any entry you still want gated there. Setting it for one + # channel of a family and not its sibling (whatsapp / whatsapp_call) is + # the common mistake; see the notes on channel_overrides below. skill_only_tools: [] enable_transient_ack: true # emit ack(s) only on COMPLEX turns (skill or multiple tools) first_emit_timeout_ms: 1200 # delay before the FIRST ack @@ -164,6 +167,22 @@ actions: # -- Voice-call loop profile: WhatsApp calls run a tighter/faster loop # than chat (shorter history prompt, tick + wall-clock caps, per-tool # timeout, and a shorter spoken-reply cap). Chat turns are untouched. + # + # TWO RULES, both silent when you get them wrong (see + # docs/ORCHESTRATOR.md "Per-channel overrides"): + # + # 1. LIST KNOBS REPLACE, THEY DO NOT MERGE. pinned_tools, denied_tools + # and skill_only_tools in a block stand in for the action-level list + # on that channel — anything you still want must be repeated inside + # the block. This is why a config that works with channel_overrides + # commented out can stop working once it is uncommented. An explicit + # [] means "none here", not "fall back". Scalars just take the value. + # + # 2. KEYS MATCH visitor.channel EXACTLY. No prefixes, no aliases: + # `whatsapp` does NOT cover `whatsapp_call` — voice is its own + # channel string. A mis-keyed block no-ops with nothing logged, which + # reads as "channel overrides are broken" rather than as a typo. + # Both blocks below are spelled out for exactly that reason. channel_overrides: # Pin the Flow send so signup/appointment intents work turn-1 under lean # surfacing (gpt-4.1-nano often skips use_skill / find_tool) — on the diff --git a/tests/wire/test_channel_overrides_replace.py b/tests/wire/test_channel_overrides_replace.py new file mode 100644 index 00000000..874104a2 --- /dev/null +++ b/tests/wire/test_channel_overrides_replace.py @@ -0,0 +1,204 @@ +"""``channel_overrides`` semantics, asserted from YAML through a real graph. + +The orchestrator suites set ``ex.channel_overrides = {...}`` in Python. That +pins the *resolver*; it never exercises the trip a real deployment takes — +agent.yaml → bootstrap → persisted attribute → resolution. A loader that dropped +or reshaped the nested block would leave every one of those tests green while +the feature did nothing in production. + +Two properties are pinned here because both present as "channel overrides are +broken" and neither raises: + +- list knobs (``pinned_tools`` / ``denied_tools`` / ``skill_only_tools``) + **REPLACE** the action-level list on that channel rather than merging, so a + config that worked with the block commented out can stop working when it is + uncommented +- override keys match ``visitor.channel`` **exactly** — ``whatsapp`` does not + cover ``whatsapp_call`` + +See docs/ORCHESTRATOR.md "Per-channel overrides". +""" + +from __future__ import annotations + +import textwrap +from pathlib import Path +from types import SimpleNamespace +from typing import Any + +import pytest + +APP_YAML = """ +app: channel_override_wire +context: + name: Channel Override Wire Test + description: boots a real graph to assert per-channel resolution +config: + database: + type: json + path: ./chan_jvdb + logging: + enabled: false + +agents: + - jvagent/chan_agent +""" + +AGENT_YAML = """ +agent: jvagent/chan_agent +version: 1.0.0 +author: tests +jvagent: ~0.0.1 + +context: + alias: Channel Agent + role: a test agent for per-channel override resolution + description: channel-override fixture agent + enabled: true + +actions: + - action: jvagent/orchestrator + context: + enabled: true + skill_only_tools: + - "pay__*" + pinned_tools: + - "kb__search" + channel_overrides: + whatsapp: + skill_only_tools: + - "wa__*" + pinned_tools: + - "whatsapp__send_flow" + whatsapp_call: + skill_only_tools: [] + web: + history_limit: 4 + - action: jvagent/reply + context: + enabled: true +""" + + +@pytest.fixture +async def orchestrator(tmp_path, monkeypatch) -> Any: + """The orchestrator for the app above, read back out of the graph.""" + from jvagent.cli.bootstrap import bootstrap_application_graph + from jvagent.core.agents import Agents + from jvagent.core.app_context import clear_app_root, set_app_root + + monkeypatch.setenv("JVSPATIAL_ENABLE_DEFERRED_SAVES", "false") + root = Path(tmp_path) + (root / "app.yaml").write_text(textwrap.dedent(APP_YAML).strip(), "utf-8") + agent_dir = root / "agents" / "jvagent" / "chan_agent" + agent_dir.mkdir(parents=True, exist_ok=True) + (agent_dir / "agent.yaml").write_text(textwrap.dedent(AGENT_YAML).strip(), "utf-8") + + set_app_root(str(root)) + try: + await bootstrap_application_graph(update_mode="source", app_root=str(root)) + agents = await (await Agents.get()).get_connected_agents() + assert agents, "fixture bootstrapped no agents" + actions = await (await agents[0].get_actions_manager()).get_all_actions( + enabled_only=True + ) + found = next( + (a for a in actions if type(a).__name__ == "OrchestratorInteractAction"), + None, + ) + assert found is not None, "fixture found no orchestrator on the graph" + yield found + finally: + clear_app_root() + + +def _resolve(orchestrator: Any, channel: str, key: str, current: Any) -> Any: + return orchestrator._channel_cfg(SimpleNamespace(channel=channel), key, current) + + +async def test_nested_block_survives_yaml_bootstrap(orchestrator) -> None: + """The whole nested mapping must land on the attribute, not a flattened + or emptied version of it — everything below depends on this.""" + overrides = orchestrator.channel_overrides + assert set(overrides) == {"whatsapp", "whatsapp_call", "web"} + assert overrides["whatsapp"]["skill_only_tools"] == ["wa__*"] + assert overrides["whatsapp_call"]["skill_only_tools"] == [] + assert overrides["web"]["history_limit"] == 4 + assert orchestrator.skill_only_tools == ["pay__*"] + + +async def test_list_knobs_replace_rather_than_merge(orchestrator) -> None: + """The trap: the action-level entries are GONE on an overridden channel. + + An operator reading this as "adds to" ships a channel where the global + gate silently does not apply. + """ + resolved = _resolve( + orchestrator, "whatsapp", "skill_only_tools", orchestrator.skill_only_tools + ) + assert resolved == ["wa__*"] + assert "pay__*" not in resolved + + pins = _resolve(orchestrator, "whatsapp", "pinned_tools", orchestrator.pinned_tools) + assert pins == ["whatsapp__send_flow"] + assert "kb__search" not in pins + + +async def test_explicit_empty_means_none_here_not_fall_back(orchestrator) -> None: + """``[]`` gates nothing on that channel; a truthiness check gets this wrong.""" + assert ( + _resolve( + orchestrator, + "whatsapp_call", + "skill_only_tools", + orchestrator.skill_only_tools, + ) + == [] + ) + + +async def test_channel_without_that_key_falls_back(orchestrator) -> None: + """``web`` overrides only history_limit, so the gate stays action-level.""" + assert _resolve( + orchestrator, "web", "skill_only_tools", orchestrator.skill_only_tools + ) == ["pay__*"] + assert _resolve(orchestrator, "web", "history_limit", 12) == 4 + + +async def test_keys_match_the_channel_string_exactly(orchestrator) -> None: + """No prefix matching: a sibling channel does not inherit the block. + + ``whatsapp`` vs ``whatsapp_call`` is the pairing that actually ships, and a + mis-keyed block no-ops silently rather than erroring. + """ + whatsapp = _resolve( + orchestrator, "whatsapp", "pinned_tools", orchestrator.pinned_tools + ) + assert whatsapp == ["whatsapp__send_flow"] + + # A channel that shares the 'whatsapp' prefix but has NO block of its own. + # This is the case that actually detects prefix matching: a sibling with its + # own block resolves by exact hit, so the fallback path never runs and the + # assertion proves nothing. + unblocked_sibling = _resolve( + orchestrator, "whatsapp_media", "pinned_tools", orchestrator.pinned_tools + ) + assert unblocked_sibling == [ + "kb__search" + ], "whatsapp_media must fall back to action-level, not inherit whatsapp's block" + + # And one with a block of its own keeps that block, not the prefix's. + assert ( + _resolve( + orchestrator, + "whatsapp_call", + "skill_only_tools", + orchestrator.skill_only_tools, + ) + == [] + ) + + unknown = _resolve( + orchestrator, "telegram", "skill_only_tools", orchestrator.skill_only_tools + ) + assert unknown == ["pay__*"]