From 7b8a3caa6ae3741d4af5690de97f3fb87f51d30a Mon Sep 17 00:00:00 2001 From: Eldon Marks Date: Wed, 5 Aug 2026 16:45:28 -0400 Subject: [PATCH 1/2] test(orchestrator): pin per-channel resolution of skill_only_tools Covers the shapes an operator actually hits and that the existing suite missed: an explicit empty override gates nothing (rather than falling back), an override gates its own channel when the action-level list is empty, alternating channels do not leak state, an unknown channel falls back, the channel-resolved list drives the lean pre-surface pool, and a per-channel deny still beats the gate. The load-bearing one is exact-key matching: channel_overrides is looked up by visitor.channel verbatim, so a 'whatsapp' block does not cover a 'whatsapp_call' voice turn. A mis-keyed override silently no-ops and the action-level list applies, which presents as 'channel overrides are broken' rather than as a typo. Documented in docs/ORCHESTRATOR.md and the config-key reference. --- .planning/reference/configuration-keys.md | 2 +- docs/ORCHESTRATOR.md | 1 + .../test_skill_only_channel_overrides.py | 221 ++++++++++++++++++ 3 files changed, 223 insertions(+), 1 deletion(-) create mode 100644 tests/action/orchestrator/test_skill_only_channel_overrides.py diff --git a/.planning/reference/configuration-keys.md b/.planning/reference/configuration-keys.md index e082024b..bffc54c9 100644 --- a/.planning/reference/configuration-keys.md +++ b/.planning/reference/configuration-keys.md @@ -258,7 +258,7 @@ Only bites with a reasoning-capable model; the `gpt-4o-mini` default ignores rea | `lean_presurface_k` | `6` | in lean mode, how many capability tools to pre-surface each turn by relevance to the user's message (token overlap, no model call), so common single-intent turns need no `find_tool` round-trip. **`0` = essentials-only** (see recipe below) | | `pinned_tools` | `[]` | tool-name globs (e.g. `["filing__*", "case__create"]`) kept **visible every turn even under lean** — for capabilities that must be callable turn-1 regardless of phrasing, without disabling lean for the rest. Skill-native equivalent: a `SKILL.md` with `always-active: true` pins its `allowed-tools` | | `denied_tools` | `[]` | tool-name globs (e.g. `["file_interface__*", "code_execution__bash"]`) **hard-removed** from the Orchestrator surface — not listed, not `find_tool`-reachable, not dispatchable. Mirrors `denied_skills` for action/MCP tools that ride in via enabled actions. Egress/meta (`reply`/`respond`/`find_*`/`load_tool`/`use_skill`) cannot be denied. Channel-overridable via `channel_overrides.denied_tools` (replaces the action-level list). Prefer this over disabling a whole action when only some of its tools should be unavailable | -| `skill_only_tools` | `[]` | tool-name globs (e.g. `["payments__*"]`) callable **only while a skill that declares them in its `allowed-tools` is active** (activated via `use_skill`, holding the turn-lock, or `always-active: true`). While **closed**: kept off the prompt and out of the lean pre-surface pool, shown by `find_tool` annotated `(via skill: …)`, and a direct call is refused with a steer to `use_skill` instead of running. Once **open** (an owner is active) the tool is listed and unannotated like any other. Fail-closed: a gated tool no available skill declares is uncallable (warned at assembly), and a glob matching no tool is warned. `denied_tools` wins over this; a `pinned_tools` match cannot un-gate. Egress/meta cannot be gated. Channel-overridable via `channel_overrides.skill_only_tools` (replaces the action-level list). ADR-0043 | +| `skill_only_tools` | `[]` | tool-name globs (e.g. `["payments__*"]`) callable **only while a skill that declares them in its `allowed-tools` is active** (activated via `use_skill`, holding the turn-lock, or `always-active: true`). While **closed**: kept off the prompt and out of the lean pre-surface pool, shown by `find_tool` annotated `(via skill: …)`, and a direct call is refused with a steer to `use_skill` instead of running. Once **open** (an owner is active) the tool is listed and unannotated like any other. Fail-closed: a gated tool no available skill declares is uncallable (warned at assembly), and a glob matching no tool is warned. `denied_tools` wins over this; a `pinned_tools` match cannot un-gate. Egress/meta cannot be gated. Channel-overridable via `channel_overrides.skill_only_tools` (replaces the action-level list — an explicit `[]` means "gate nothing here", not "fall back"). **The override key must match `visitor.channel` exactly** — a `whatsapp` block does NOT cover a `whatsapp_call` (voice) turn, and a mis-keyed override silently no-ops. ADR-0043 | | `max_observations_in_prompt` | `12` | how many of this turn's tool results replay into the loop prompt (most recent first). Raise for long agentic turns whose later steps depend on early findings; `0` replays all (size caps still apply) | | `observation_max_chars` | `4000` | max characters of a **recent** tool result replayed into the loop prompt (middle-out elided, and marked). `0` disables | | `stale_observation_max_chars` | `600` | max characters of an **older** result (beyond `observation_full_recent`). Older results matter as "what happened", not as payload. `0` disables | diff --git a/docs/ORCHESTRATOR.md b/docs/ORCHESTRATOR.md index cf7d50fc..e532dc93 100644 --- a/docs/ORCHESTRATOR.md +++ b/docs/ORCHESTRATOR.md @@ -101,6 +101,7 @@ Two further knobs/notes: - **Always-visible pins (turn-1 immediacy).** The relevance pre-surface is lexical, so a tool that must be callable on the first turn *regardless of phrasing* (e.g. a filing tool) can fall behind a `find_tool` round-trip. Rather than un-leaning the whole surface with `lean_tool_threshold: 0`, pin just the few that need it — `pinned_tools: ["filing__*"]` (tool-name globs) or a `SKILL.md` with `always-active: true` (pins that skill's `allowed-tools`). Both are merged into the visible set *after* the lean policy, every turn, and default off. (`always-active` previously did nothing to tool visibility under the orchestrator — it now pins, per [ADR-0018 §5](../.planning/adr/0018-lean-tool-surfacing.md).) - **Hard deny (capability gate).** `denied_tools: ["file_interface__*"]` (fnmatch globs) removes matching tools from the assembled surface entirely — not listed, not discoverable via `find_tool`, not dispatchable. Applied after pins so a deny wins. Mirrors `denied_skills`. Egress and catalog meta-tools cannot be denied. Channel-overridable via `channel_overrides.denied_tools`. Prefer this over disabling a whole action when only some of its tools should be unavailable; MCP servers still have their own per-server `denied_tools`. - **Skill-only (procedure gate).** `skill_only_tools: ["payments__*"]` (fnmatch globs) makes matching tools callable **only while a skill that declares them in its `allowed-tools` is active** — activated via `use_skill`, holding the turn-lock, or `always-active: true`. While the gate is **closed** the tool is kept off the prompt, `find_tool` shows it annotated `(via skill: checkout)`, and a direct call is refused with a steer to `use_skill` rather than running; gated names are also excluded from the lean pre-surface pool, so gating a family never shrinks the visible long tail. Once an owner is active the gate is **open** and the tool behaves normally — listed, and unannotated, so the model is never steered into a redundant `use_skill` for a tool it can already call. A gated tool no available skill declares is uncallable (fail closed, warned at assembly); a glob matching no tool at all is warned too. Precedence: `denied_tools` wins over this, and this wins over `pinned_tools` (a pin cannot un-gate). Egress and catalog meta-tools cannot be gated. Channel-overridable via `channel_overrides.skill_only_tools`. Use this when a capability must always run inside its SOP — a payment charge, a destructive write — where hard deny would also break the skill that legitimately needs it. See [ADR-0043](../.planning/adr/0043-skill-only-tools.md). +- **Channel overrides key on the EXACT channel string.** `channel_overrides` is looked up by `visitor.channel` verbatim — no prefix matching, no aliasing. A voice turn runs on its own channel (`whatsapp_call`), which is **not** covered by a `whatsapp` block, so an override written under the wrong key silently no-ops and the action-level list applies instead. This bites hardest on the subtractive knobs (`denied_tools`, `skill_only_tools`), where the symptom is "the override does nothing" rather than an error. When an override appears to be ignored, check the channel string first. Note also that each override **replaces** the action-level list for that key rather than merging with it — an explicit `[]` means "nothing here", not "fall back". Pinned by `tests/action/orchestrator/test_skill_only_channel_overrides.py`. - **Plan-aware pre-surface.** When `planning: true` and a multi-step plan is in progress, the lean relevance signal is the user's message **plus the active plan's checklist**. A plan resumed on a low-signal turn ("Well?", "continue") would otherwise surface nothing and force `find_tool` round-trips for the tools the next step needs (e.g. `pageindex__assimilate` for "add to knowledge base"); folding the checklist into the signal keeps those tools visible without a round-trip. - **Naming a hidden tool just runs it.** If the model names a tool that's real but lean-hidden, the loop auto-promotes it to visible and dispatches it (an implicit `load_tool`) rather than rejecting it — hiding is a prompt-size optimization, not a capability gate. `find_tool` is for *discovering* a tool whose exact name you don't know; an unknown/hallucinated name is bounced there with examples. - **Tolerant decision parsing.** The loop normalizer salvages the common malformed shapes a model emits so a step isn't wasted: text in `answer`/`content` instead of `args.text`; a skill addressed as if it were a tool (`{"tool":"research"}` → `use_skill`); and a **flattened** call where tool args sit at the decision top level instead of under `args` (`{"tool":"update_plan","steps":[...]}`) — when no `args` object is supplied, the non-reserved top-level keys are folded in. `update_plan` additionally accepts its list under many key aliases / a bare string / a single inline step. diff --git a/tests/action/orchestrator/test_skill_only_channel_overrides.py b/tests/action/orchestrator/test_skill_only_channel_overrides.py new file mode 100644 index 00000000..925da720 --- /dev/null +++ b/tests/action/orchestrator/test_skill_only_channel_overrides.py @@ -0,0 +1,221 @@ +"""Per-channel resolution of ``skill_only_tools`` (ADR-0043 + ADR-0032). + +``channel_overrides.skill_only_tools`` REPLACES the action-level list on that +channel. The existing suite covers the "different list" case; these pin the +shapes an operator actually hits in the field and that a naive resolver gets +wrong: + +- an **explicit empty** override means "gate nothing here" — not "fall back to + the action-level list" (a truthiness-based resolver would get this backwards) +- an empty action-level list with a per-channel override gates *only* there +- resolution is per-turn, so alternating channels never leak state +- an unknown channel falls back to the action-level list +- the channel-resolved list — not the action-level one — drives the lean + pre-surface candidate pool +- override keys match ``visitor.channel`` EXACTLY: ``whatsapp`` does not cover + ``whatsapp_call`` + +That last one is the trap. Voice runs on its own channel string, so an override +written under ``whatsapp`` silently no-ops on a ``whatsapp_call`` turn and the +action-level list applies instead — which reads as "channel overrides are +broken" rather than as a typo. +""" + +from __future__ import annotations + +from types import SimpleNamespace +from typing import Any, List, Set, Tuple + +import pytest + +pytestmark = pytest.mark.asyncio + + +def _doc(name: str, tools: Any = (), always_active: bool = False) -> SimpleNamespace: + """A minimal SkillDoc stand-in (only the fields the gate reads).""" + return SimpleNamespace( + name=name, requires_tools=tuple(tools), always_active=always_active + ) + + +class _ToolsAction: + """A plain action exposing namespaced capability tools.""" + + def __init__(self, names_descs: List[Tuple[str, str]]) -> None: + self._t = [ + SimpleNamespace(name=n, description=d, call=None) for n, d in names_descs + ] + + async def get_tools(self) -> List[Any]: + return self._t + + +_PAY = [ + ("pay__charge", "Charge a saved payment method."), + ("pay__refund", "Refund a settled charge."), + ("kb__search", "Search the knowledge base."), +] + + +def _wire_skills(monkeypatch: pytest.MonkeyPatch, ex: Any, docs: List[Any]) -> None: + """Surface ``docs`` as this agent's skills without touching the resolver.""" + monkeypatch.setattr(ex, "_discover_skills", lambda _agent: list(docs)) + monkeypatch.setattr( + "jvagent.action.orchestrator.skill_tasks.compose_skill_activate_hooks", + lambda *a, **k: (None, None), + ) + + +async def _gate_state(ex: Any, make_visitor: Any, channel: str) -> Tuple[bool, bool]: + """Assemble a turn on *channel*; return ``(refused, visible)`` for pay__charge. + + ``refused`` is the load-bearing half — it is the gate itself, observed by + calling the tool rather than by inspecting config. + """ + visible: Set[str] = set() + tools = await ex._assemble_tools( + make_visitor(utterance="charge me", channel=channel), + [], + visible, + None, + "charge me", + None, + {}, + ) + observation = await tools["pay__charge"].run({}) + return ("only available inside a skill" in observation), ("pay__charge" in visible) + + +async def test_empty_override_ungates_that_channel( + monkeypatch, make_orchestrator, make_visitor +): + """An explicit ``[]`` override gates nothing there — it does not fall back.""" + ex = make_orchestrator(actions=[_ToolsAction(_PAY)]) + ex.lean_tool_threshold = 0 # list everything, so visibility is unambiguous + ex.skill_only_tools = ["pay__*"] + ex.channel_overrides = {"voice": {"skill_only_tools": []}} + _wire_skills(monkeypatch, ex, [_doc("checkout", ["pay__charge"])]) + + assert await _gate_state(ex, make_visitor, "voice") == (False, True) + assert await _gate_state(ex, make_visitor, "web") == (True, False) + + +async def test_override_gates_when_action_level_is_empty( + monkeypatch, make_orchestrator, make_visitor +): + """Nothing gated globally; the override gates its own channel only.""" + ex = make_orchestrator(actions=[_ToolsAction(_PAY)]) + ex.lean_tool_threshold = 0 + ex.skill_only_tools = [] + ex.channel_overrides = {"voice": {"skill_only_tools": ["pay__*"]}} + _wire_skills(monkeypatch, ex, [_doc("checkout", ["pay__charge"])]) + + assert await _gate_state(ex, make_visitor, "voice") == (True, False) + assert await _gate_state(ex, make_visitor, "web") == (False, True) + + +async def test_alternating_channels_do_not_leak_state( + monkeypatch, make_orchestrator, make_visitor +): + """Resolution is per-turn. A gated turn must not poison the next channel's + surface, nor be poisoned by it — the assembled sets and the per-turn cache + are shared machinery, so this is worth pinning rather than assuming.""" + ex = make_orchestrator(actions=[_ToolsAction(_PAY)]) + ex.lean_tool_threshold = 0 + ex.skill_only_tools = [] + ex.channel_overrides = {"voice": {"skill_only_tools": ["pay__*"]}} + _wire_skills(monkeypatch, ex, [_doc("checkout", ["pay__charge"])]) + + first = await _gate_state(ex, make_visitor, "voice") + between = await _gate_state(ex, make_visitor, "web") + again = await _gate_state(ex, make_visitor, "voice") + + assert first == (True, False) + assert between == (False, True) + assert again == first + + +async def test_unknown_channel_falls_back_to_action_level( + monkeypatch, make_orchestrator, make_visitor +): + ex = make_orchestrator(actions=[_ToolsAction(_PAY)]) + ex.lean_tool_threshold = 0 + ex.skill_only_tools = ["pay__*"] + ex.channel_overrides = {"voice": {"skill_only_tools": ["kb__*"]}} + _wire_skills(monkeypatch, ex, [_doc("checkout", ["pay__charge", "kb__search"])]) + + assert await _gate_state(ex, make_visitor, "telegram") == (True, False) + assert await _gate_state(ex, make_visitor, "default") == (True, False) + + +async def test_override_key_must_match_the_channel_exactly( + monkeypatch, make_orchestrator, make_visitor +): + """``whatsapp`` does NOT cover ``whatsapp_call``. + + Voice runs on its own channel string, so an override written under the chat + channel silently no-ops on a voice turn and the action-level list applies. + No prefix matching, no aliasing — this is the field trap, pinned so nobody + "fixes" it into fuzzy matching by accident. + """ + ex = make_orchestrator(actions=[_ToolsAction(_PAY)]) + ex.lean_tool_threshold = 0 + ex.skill_only_tools = ["pay__*"] + ex.channel_overrides = {"whatsapp": {"skill_only_tools": []}} + _wire_skills(monkeypatch, ex, [_doc("checkout", ["pay__charge"])]) + + # The override applies on the channel it names... + assert await _gate_state(ex, make_visitor, "whatsapp") == (False, True) + # ...and NOT on the adjacent voice channel, which keeps the action-level gate. + assert await _gate_state(ex, make_visitor, "whatsapp_call") == (True, False) + + +async def test_channel_resolved_list_drives_the_lean_pool( + monkeypatch, make_orchestrator, make_visitor +): + """Under lean, gated names are excluded from the pre-surface candidate pool. + That exclusion must follow the CHANNEL-resolved list, not the action-level + one — otherwise a channel that ungates a tool still can't surface it.""" + many = _PAY + [(f"misc__t{i:02d}", f"Miscellaneous capability {i}") for i in range(20)] + ex = make_orchestrator(actions=[_ToolsAction(many)]) + ex.lean_tool_threshold = 5 # force lean on + ex.lean_presurface_k = 3 + ex.skill_only_tools = [] + ex.channel_overrides = {"voice": {"skill_only_tools": ["pay__*"]}} + _wire_skills(monkeypatch, ex, [_doc("checkout", ["pay__charge"])]) + + # Gated on voice: refused, and kept out of the lean pre-surface set. + assert await _gate_state(ex, make_visitor, "voice") == (True, False) + # Ungated on web: relevance ("charge me") pre-surfaces it normally. + assert await _gate_state(ex, make_visitor, "web") == (False, True) + + +async def test_denied_channel_override_still_beats_the_gate( + monkeypatch, make_orchestrator, make_visitor +): + """Precedence holds per-channel: a channel that denies a gated tool removes + it from the surface entirely rather than merely gating it.""" + ex = make_orchestrator(actions=[_ToolsAction(_PAY)]) + ex.lean_tool_threshold = 0 + ex.skill_only_tools = ["pay__*"] + ex.denied_tools = [] + ex.channel_overrides = {"voice": {"denied_tools": ["pay__charge"]}} + # ``checkout`` owns BOTH gated tools, so the sibling below is a genuine + # closed gate rather than an orphan — this test is about deny-vs-gate. + _wire_skills(monkeypatch, ex, [_doc("checkout", ["pay__charge", "pay__refund"])]) + + visible: Set[str] = set() + tools = await ex._assemble_tools( + make_visitor(utterance="charge me", channel="voice"), + [], + visible, + None, + "charge me", + None, + {}, + ) + assert "pay__charge" not in tools + assert "pay__charge" not in visible + # The sibling gated tool is untouched by the deny and still gated. + assert "pay__refund" in tools + assert "only available inside a skill" in await tools["pay__refund"].run({}) From 9da2db75a31f659f519bea925f497ec3fb2b4bc3 Mon Sep 17 00:00:00 2001 From: Eldon Marks Date: Wed, 5 Aug 2026 17:36:47 -0400 Subject: [PATCH 2/2] feat(validate): advise when a channel override misses its sibling channel MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit channel_overrides is resolved by the exact visitor.channel string, so a block written for 'whatsapp' does nothing on a 'whatsapp_call' voice turn and the action-level value applies silently. Both are valid channel names, so no key-validity check can catch it; the only signal is that one member of a channel family is configured while its reachable sibling is not. Fires only when the sibling is genuinely reachable (its providing action is enabled on the agent) and only for keys whose absence changes behavior silently: skill_only_tools, denied_tools, pinned_tools. Per-channel divergence in history_limit, ack knobs and system_prompt_extra is normal and is ignored. Adds an 'advisory' severity to AgentYamlWarning. Advisories are printed but do not affect the exit code, so a heuristic lint cannot break an existing pipeline; 'jvagent validate --strict' promotes them to failures. CHANNEL_PROVIDERS keys on each action's published package.name, not its directory name (jvagent/action/whatsapp_voice/ publishes as jvagent/whatsapp_voice_action) — a test pins that against the real info.yaml, because a wrong ref makes the lint silently never fire while every behavioral test still passes. --- ...-04-channel-overrides-validation-design.md | 272 ++++++++++++++++++ CHANGELOG.md | 17 ++ docs/ORCHESTRATOR.md | 2 +- jvagent/cli/main.py | 2 +- jvagent/cli/validate.py | 35 ++- jvagent/core/agent_yaml_validator.py | 95 +++++- jvagent/core/channel.py | 50 +++- .../test_skill_only_channel_overrides.py | 4 +- tests/cli/test_validate_advisories.py | 80 ++++++ tests/core/test_channel_override_coverage.py | 253 ++++++++++++++++ 10 files changed, 799 insertions(+), 11 deletions(-) create mode 100644 .planning/specs/2026-08-04-channel-overrides-validation-design.md create mode 100644 tests/cli/test_validate_advisories.py create mode 100644 tests/core/test_channel_override_coverage.py diff --git a/.planning/specs/2026-08-04-channel-overrides-validation-design.md b/.planning/specs/2026-08-04-channel-overrides-validation-design.md new file mode 100644 index 00000000..1fe54972 --- /dev/null +++ b/.planning/specs/2026-08-04-channel-overrides-validation-design.md @@ -0,0 +1,272 @@ +# Validate-time checks for `channel_overrides` keys — design + +**Date:** 2026-08-04 +**Status:** Partially implemented — **Check B shipped**; Check A was reviewed and +dropped as low-yield (see §1: it would not have caught the incident). Also +dropped with it: the known-channel set (§5.1) and the `provides_channels` action +declaration, neither of which Check B needs — reachability is determined from the +agent's own `actions:` refs, since `validate_agent_yaml` operates on the YAML +dict and never loads Action classes. §9 records the as-built deltas. +**Scope:** `jvagent/core/channel.py` (known-channel registry), `jvagent/core/agent_yaml_validator.py` (the checks), `jvagent/cli/validate.py` (severity plumbing), `jvagent/action/base.py` (optional `provides_channels` declaration), channel-adapter actions (declare their channels), `tests/core/`, `tests/cli/`, docs. +**Relation:** Follows the `whatsapp` / `whatsapp_call` misconfiguration found while investigating [ADR-0043](../adr/0043-skill-only-tools.md) channel overrides. Applies to every `channel_overrides` key, not just `skill_only_tools`. + +--- + +## 1. Context — and a correction to the premise + +`channel_overrides` is resolved by `_channel_cfg` +([`orchestrator_interact_action.py:2671`](../../jvagent/action/orchestrator/orchestrator_interact_action.py)), +which looks up `visitor.channel` **verbatim**. A block keyed on a channel that +never occurs silently no-ops and the action-level value applies. + +The incident that prompted this: an operator put `skill_only_tools` under +`whatsapp`, but voice turns arrive on `whatsapp_call`. The override never +applied. + +**The check as literally requested — "validate that `channel_overrides` keys are +known channels" — would not have caught it.** `whatsapp` is a perfectly valid +channel; the key was valid and the *intent* was wrong. Key-validity checking +catches `whatsap` and `whats_app`. It does not catch a correct key on the wrong +channel, which is the failure that actually happened and is the more likely one, +because both keys are real and the example app ships blocks for both. + +So this spec proposes two checks. **Check A** is the requested key lint. **Check +B** is the sibling-coverage lint that catches the reported incident. B is the +one that pays for itself; A is cheap and catches a different, real class of typo. + +### 1.1 The hard constraint: channels are an open set + +There is no channel registry to validate against: + +- `/agents/{id}/interact` takes `channel` as a **free-form query parameter** + ([`interact/endpoints.py:533`](../../jvagent/action/interact/endpoints.py)), + so any caller can invent one. +- `normalize_channel` ([`core/channel.py:10`](../../jvagent/core/channel.py)) + only folds `None`/`""`/`web` → `default`; everything else passes through. +- Adapters emit string literals scattered across modules — `whatsapp` + ([`whatsapp_adapter.py`](../../jvagent/action/whatsapp/whatsapp_adapter.py)), + `email` ([`email_adapter.py`](../../jvagent/action/email_action/email_adapter.py)), + `messenger` ([`messenger_adapter.py`](../../jvagent/action/facebook_action/messenger_adapter.py)). +- `whatsapp_call` is not emitted by any adapter literal at all — it arrives from + jvvoice through the interact endpoint's `channel` parameter, and is known to + the codebase only as a bare string in + [`whatsapp_action.py:1527`](../../jvagent/action/whatsapp/whatsapp_action.py) + (`_OUTBOUND_WA_CHANNELS`) and an orchestrator default. + +**Consequence:** an unknown key can never be an error, only an advisory. A custom +deployment with a bespoke channel is legitimate and must not be failed. + +### 1.2 The second constraint: warnings currently fail CI + +`run_validate` "returns 1 if any warning-level issue is found (suitable for CI)" +([`cli/validate.py:13`](../../jvagent/cli/validate.py)). Every existing +`AgentYamlWarning` is CI-fatal. Adding these checks at that severity would break +the build of any app that has a deliberately-unused override block — an +unacceptable upgrade experience for an advisory lint. + +## 2. Goals / non-goals + +**Goals** + +- Surface a mis-keyed `channel_overrides` block at `jvagent validate` time + instead of as silent no-op behavior in production. +- Catch the specific `whatsapp` / `whatsapp_call` class of error (Check B). +- Never fail an app for a channel the validator merely doesn't recognize. +- Give the operator a concrete next action, not just "unknown channel". + +**Non-goals** + +- A closed-world channel registry. Channels stay open by design (§1.1). +- Runtime enforcement or runtime warnings. This is a config-time lint. +- Changing `_channel_cfg` resolution semantics — exact-key matching is correct + and is now pinned by `tests/action/orchestrator/test_skill_only_channel_overrides.py`. + Fuzzy/prefix matching is explicitly rejected (§7). +- Validating override *values* (that's the existing per-key validation). + +## 3. Locked decisions + +| # | Decision | Choice | +|---|---|---| +| 1 | Severity | **New `advisory` severity** that does not fail CI by default. `jvagent validate --strict` escalates advisories to warnings (exit 1). Existing warnings are unchanged. | +| 2 | Known-channel set | Union of three sources (§5.1): a first-party constant, channels declared by enabled actions, and channels referenced elsewhere in the same `agent.yaml`. Open-world: an unrecognized channel is advisory-only. | +| 3 | Check A trigger | An override key not in the known set. Message includes a `difflib` close-match suggestion when one exists. | +| 4 | Check B trigger | An override key sets a knob for one member of a known **channel family** while a sibling member is reachable (its providing action is enabled) and has no block setting that same knob. | +| 5 | Channel families | Declared alongside the first-party constant, seeded with `{whatsapp, whatsapp_call}`. Extensible by actions via the same `provides_channels` declaration. | +| 6 | Which knobs Check B covers | Only knobs whose absence changes behavior silently: `skill_only_tools`, `denied_tools`, `pinned_tools`. Not `history_limit`, ack knobs, or `system_prompt_extra`, where per-channel divergence is normal and intentional. | +| 7 | Action declaration | A new optional `provides_channels: Tuple[str, ...]` class attribute on `Action`, defaulting to `()`. Adapters declare theirs. Purely additive; no action is required to implement it. | + +## 4. Worked example + +```yaml +# agent.yaml — the incident, reproduced +actions: + - action: jvagent/orchestrator + context: + skill_only_tools: ["payments__*"] + channel_overrides: + whatsapp: + skill_only_tools: [] # meant to ungate voice +``` + +With `jvagent/whatsapp_voice` enabled, `jvagent validate` emits: + +``` +advisory actions[0].context.channel_overrides: + 'skill_only_tools' is overridden for channel 'whatsapp' but not for its + sibling 'whatsapp_call', which is reachable on this agent (provided by + WhatsAppVoiceAction). Turns on 'whatsapp_call' will use the action-level + skill_only_tools instead. Add a 'whatsapp_call' block if that is not intended. +``` + +And for a genuine typo: + +``` +advisory actions[0].context.channel_overrides: + channel 'whatsap' is not a channel any enabled action provides, and is not + referenced elsewhere in this agent.yaml. Did you mean 'whatsapp'? + (Custom channels are supported — this is advisory only.) +``` + +## 5. Detailed design + +### 5.1 The known-channel set + +Built per agent, as the union of: + +1. **First-party constant** — `KNOWN_CHANNELS` in + [`jvagent/core/channel.py`](../../jvagent/core/channel.py), the natural home + (it already owns `normalize_channel`): `default`, `whatsapp`, + `whatsapp_call`, `email`, `messenger`. This is documentation as much as + validation — today those strings exist only as scattered literals, which is + itself part of why the incident happened. +2. **Action-declared** — the union of `provides_channels` over the actions the + `agent.yaml` enables. This is what makes the check correct for third-party + adapters rather than only first-party ones. +3. **Self-referenced** — channels named elsewhere in the same `agent.yaml`: + `voice_ack_channels`, and any skill `allowed-channels` / `denied-channels` + reachable from this agent. Rationale: an operator who uses a custom channel + consistently across their config has demonstrated intent, and should not be + nagged. + +Source 3 is what keeps Check A's false-positive rate near zero without a +registry. + +### 5.2 Check A — unknown key + +For each key in `channel_overrides`, if it is not in the known set, emit an +advisory. Compute a suggestion with `difflib.get_close_matches(key, known, n=1, +cutoff=0.8)`; include it when found. The message must state that custom channels +are supported, so the operator does not "fix" a deliberate custom channel. + +Note `normalize_channel` folds `web` → `default`: a block keyed `web` would never +match, since `visitor.channel` is normalized before lookup. Check A should treat +`web` as a **special case with a definite fix** ("use `default`"), not a generic +unknown — it is a guaranteed-dead key, not a maybe. + +### 5.3 Check B — sibling coverage + +For each channel family (§3 decision #5), for each knob in the covered set +(#6): if some family member's block sets that knob, and another family member is +**reachable** on this agent but has no block setting it, emit an advisory naming +the uncovered sibling and what will happen instead. + +"Reachable" means an enabled action declares that channel via +`provides_channels`. This matters: an agent with no voice action should not be +told about `whatsapp_call`. Getting this wrong turns a useful lint into noise, +so the check is deliberately conservative — no declaration, no advisory. + +The message must state the *consequence* ("turns on X will use the action-level +value"), not merely the omission. The omission is often correct; the consequence +is what the operator needs in order to judge. + +### 5.4 Severity plumbing + +`AgentYamlWarning` gains a `severity: str = "warning"` field. `_mk` keeps its +current default so every existing call site is unchanged. `run_validate` +partitions results: warnings → exit 1 as today; advisories → printed, exit +unaffected. `--strict` promotes advisories to warnings. + +This is the smallest change that avoids breaking existing CI (§1.2), and it +gives future advisory-grade checks a home rather than forcing each one to choose +between "silent" and "CI-fatal". + +## 6. Testing + +`tests/core/test_channel_overrides_validation.py`: + +1. Unknown key → advisory, not warning; `run_validate` still exits 0 +2. Unknown key with a near-miss → advisory includes the suggestion +3. `web` key → advisory naming `default` as the fix +4. Custom channel referenced in `voice_ack_channels` → **no** advisory +5. Custom channel declared by an enabled action's `provides_channels` → no advisory +6. Check B: `skill_only_tools` on `whatsapp`, voice action enabled, no + `whatsapp_call` block → advisory naming the sibling and the consequence +7. Check B: same config, voice action **not** enabled → no advisory +8. Check B: both siblings set the knob → no advisory +9. Check B: only a non-covered knob (`history_limit`) differs → no advisory +10. `--strict` promotes advisories to exit 1 +11. An app with no `channel_overrides` produces no advisories (no regression) + +`tests/cli/test_validate_strict.py`: exit-code matrix for warning × advisory × +`--strict`. + +## 7. Alternatives considered + +- **Make `_channel_cfg` match prefixes or aliases** (`whatsapp` covers + `whatsapp_call`). Rejected outright: it would silently change the meaning of + every existing config, and the separation is deliberate — a voice turn genuinely + wants different knobs from a chat turn. It is now pinned against by + `test_override_key_must_match_the_channel_exactly`. +- **Error on unknown keys.** Impossible under §1.1 without breaking custom + channels, which are a supported deployment shape. +- **Runtime warning on first turn for a channel with no override while a sibling + has one.** Catches what config-time analysis can't (channels that actually + occur), but fires in production for a config-time mistake, needs per-process + dedup state, and says nothing until the miss already happened. Worth + reconsidering only if the config-time checks prove insufficient in the field. +- **A closed channel registry actions must register into.** The clean long-term + model, but a breaking change for third-party adapters and far more than this + problem justifies. `provides_channels` is the additive subset of that idea. +- **Check A alone** (the literal request). Cheap, but would not have caught the + incident that motivated this — see §1. + +## 8. Risks + +| Risk | Mitigation | +|---|---| +| Advisory noise trains operators to ignore output | Check B is gated on the sibling being *reachable*; Check A is suppressed by any use of the channel elsewhere in the config. Both fire only on a concrete, actionable mismatch. | +| `KNOWN_CHANNELS` drifts as adapters are added | Source 2 (`provides_channels`) is the real mechanism; the constant is a convenience for first-party strings. A missing entry degrades to an advisory, never a failure. | +| `--strict` in someone's CI turns advisories fatal on upgrade | `--strict` is new and opt-in; default behavior for existing pipelines is unchanged. | +| Check B's family list becomes a maintenance burden | Seeded with one family; extensible through the same action declaration rather than a central list. If no second family materializes, that is evidence the check should stay narrow. | + +## 9. As built + +Check B shipped; Check A did not. Deltas from §§3–6 above: + +1. **Reachability comes from `actions:` refs, not `provides_channels`.** + `validate_agent_yaml` operates on the parsed YAML dict and never loads Action + classes, so a class attribute would have been invisible to it. `CHANNEL_PROVIDERS` + maps channel → the action's published `package.name` instead. Third-party + adapters are therefore unknown to the lint and produce no advisory — fail-quiet, + which is the right default for a heuristic. + +2. **Provider refs are package names, not directory names.** `jvagent/action/whatsapp_voice/` + publishes as `jvagent/whatsapp_voice_action`. The first draft used directory + names, which made the lint silently never fire while every behavioral test still + passed — the tests used the same wrong constant as their fixtures. + `test_channel_providers_match_real_action_package_names` now reads the real + `info.yaml` files and pins the refs; it was mutation-checked against the + original bug. + +3. **The advisory is symmetric.** Gating voice but not chat is flagged the same + as chat but not voice. §5.3 implied a one-way check; there is no basis for + treating one direction as more likely. + +4. **Dropped:** `KNOWN_CHANNELS`, the difflib suggestion, the `web`→`default` + special case, and the self-referenced-channel source — all of which existed + only to serve Check A. + +Verified end-to-end against a real app configured with the original incident's +shape: the advisory fires, `jvagent validate` exits 0, `--strict` exits 1. The +bundled example app, which sets `pinned_tools` on both `whatsapp` and +`whatsapp_call`, produces no advisory. diff --git a/CHANGELOG.md b/CHANGELOG.md index ed05a822..c27441e2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,23 @@ and this project adheres to [PEP 440](https://peps.python.org/pep-0440/) / ## [Unreleased] +### Added + +- **`jvagent validate` advisory for uncovered sibling channels.** + `channel_overrides` is matched on the exact `visitor.channel` string, so a + block written for `whatsapp` silently does nothing on a `whatsapp_call` + (voice) turn and the action-level value applies. Validation now advises when + a subtractive knob (`skill_only_tools`, `denied_tools`, `pinned_tools`) is set + for one channel of a family but not for its sibling, and that sibling is + reachable (its providing action is enabled on the agent). Both keys are valid + channels, so no key-validity check could catch this. + + Advisories are a new severity: printed, but they do **not** affect the exit + code, so this cannot break an existing pipeline. `jvagent validate --strict` + promotes them to failures. Coverage: + `tests/core/test_channel_override_coverage.py`, + `tests/cli/test_validate_advisories.py`. + ### Fixed - **No WARNING when ambient core parameters are re-unioned.** diff --git a/docs/ORCHESTRATOR.md b/docs/ORCHESTRATOR.md index e532dc93..5066c3ae 100644 --- a/docs/ORCHESTRATOR.md +++ b/docs/ORCHESTRATOR.md @@ -101,7 +101,7 @@ Two further knobs/notes: - **Always-visible pins (turn-1 immediacy).** The relevance pre-surface is lexical, so a tool that must be callable on the first turn *regardless of phrasing* (e.g. a filing tool) can fall behind a `find_tool` round-trip. Rather than un-leaning the whole surface with `lean_tool_threshold: 0`, pin just the few that need it — `pinned_tools: ["filing__*"]` (tool-name globs) or a `SKILL.md` with `always-active: true` (pins that skill's `allowed-tools`). Both are merged into the visible set *after* the lean policy, every turn, and default off. (`always-active` previously did nothing to tool visibility under the orchestrator — it now pins, per [ADR-0018 §5](../.planning/adr/0018-lean-tool-surfacing.md).) - **Hard deny (capability gate).** `denied_tools: ["file_interface__*"]` (fnmatch globs) removes matching tools from the assembled surface entirely — not listed, not discoverable via `find_tool`, not dispatchable. Applied after pins so a deny wins. Mirrors `denied_skills`. Egress and catalog meta-tools cannot be denied. Channel-overridable via `channel_overrides.denied_tools`. Prefer this over disabling a whole action when only some of its tools should be unavailable; MCP servers still have their own per-server `denied_tools`. - **Skill-only (procedure gate).** `skill_only_tools: ["payments__*"]` (fnmatch globs) makes matching tools callable **only while a skill that declares them in its `allowed-tools` is active** — activated via `use_skill`, holding the turn-lock, or `always-active: true`. While the gate is **closed** the tool is kept off the prompt, `find_tool` shows it annotated `(via skill: checkout)`, and a direct call is refused with a steer to `use_skill` rather than running; gated names are also excluded from the lean pre-surface pool, so gating a family never shrinks the visible long tail. Once an owner is active the gate is **open** and the tool behaves normally — listed, and unannotated, so the model is never steered into a redundant `use_skill` for a tool it can already call. A gated tool no available skill declares is uncallable (fail closed, warned at assembly); a glob matching no tool at all is warned too. Precedence: `denied_tools` wins over this, and this wins over `pinned_tools` (a pin cannot un-gate). Egress and catalog meta-tools cannot be gated. Channel-overridable via `channel_overrides.skill_only_tools`. Use this when a capability must always run inside its SOP — a payment charge, a destructive write — where hard deny would also break the skill that legitimately needs it. See [ADR-0043](../.planning/adr/0043-skill-only-tools.md). -- **Channel overrides key on the EXACT channel string.** `channel_overrides` is looked up by `visitor.channel` verbatim — no prefix matching, no aliasing. A voice turn runs on its own channel (`whatsapp_call`), which is **not** covered by a `whatsapp` block, so an override written under the wrong key silently no-ops and the action-level list applies instead. This bites hardest on the subtractive knobs (`denied_tools`, `skill_only_tools`), where the symptom is "the override does nothing" rather than an error. When an override appears to be ignored, check the channel string first. Note also that each override **replaces** the action-level list for that key rather than merging with it — an explicit `[]` means "nothing here", not "fall back". Pinned by `tests/action/orchestrator/test_skill_only_channel_overrides.py`. +- **Channel overrides key on the EXACT channel string.** `channel_overrides` is looked up by `visitor.channel` verbatim — no prefix matching, no aliasing. A voice turn runs on its own channel (`whatsapp_call`), which is **not** covered by a `whatsapp` block, so an override written under the wrong key silently no-ops and the action-level list applies instead. This bites hardest on the subtractive knobs (`denied_tools`, `skill_only_tools`), where the symptom is "the override does nothing" rather than an error. When an override appears to be ignored, check the channel string first. Note also that each override **replaces** the action-level list for that key rather than merging with it — an explicit `[]` means "nothing here", not "fall back". Pinned by `tests/action/orchestrator/test_skill_only_channel_overrides.py`. `jvagent validate` emits an **advisory** when a subtractive knob (`skill_only_tools`, `denied_tools`, `pinned_tools`) is set for one channel of a family but not for its reachable sibling — e.g. `whatsapp` configured while `whatsapp_call` is not, with the voice action installed. Advisories are printed but do not fail validation; `jvagent validate --strict` treats them as failures. - **Plan-aware pre-surface.** When `planning: true` and a multi-step plan is in progress, the lean relevance signal is the user's message **plus the active plan's checklist**. A plan resumed on a low-signal turn ("Well?", "continue") would otherwise surface nothing and force `find_tool` round-trips for the tools the next step needs (e.g. `pageindex__assimilate` for "add to knowledge base"); folding the checklist into the signal keeps those tools visible without a round-trip. - **Naming a hidden tool just runs it.** If the model names a tool that's real but lean-hidden, the loop auto-promotes it to visible and dispatches it (an implicit `load_tool`) rather than rejecting it — hiding is a prompt-size optimization, not a capability gate. `find_tool` is for *discovering* a tool whose exact name you don't know; an unknown/hallucinated name is bounced there with examples. - **Tolerant decision parsing.** The loop normalizer salvages the common malformed shapes a model emits so a step isn't wasted: text in `answer`/`content` instead of `args.text`; a skill addressed as if it were a tool (`{"tool":"research"}` → `use_skill`); and a **flattened** call where tool args sit at the decision top level instead of under `args` (`{"tool":"update_plan","steps":[...]}`) — when no `args` object is supplied, the non-reserved top-level keys are folded in. `update_plan` additionally accepts its list under many key aliases / a bare string / a single inline step. diff --git a/jvagent/cli/main.py b/jvagent/cli/main.py index 2b3bafa7..69be45ca 100644 --- a/jvagent/cli/main.py +++ b/jvagent/cli/main.py @@ -231,7 +231,7 @@ def main() -> None: elif args[0] == "bundle": handle_bundle_command(args[1:], app_root=app_root) elif args[0] == "validate": - sys.exit(run_validate(app_root)) + sys.exit(run_validate(app_root, strict="--strict" in args)) elif args[0] == "chat": from jvagent.cli.chat import handle_chat_command diff --git a/jvagent/cli/validate.py b/jvagent/cli/validate.py index 29a8789d..4b9bda80 100644 --- a/jvagent/cli/validate.py +++ b/jvagent/cli/validate.py @@ -7,15 +7,23 @@ logger = logging.getLogger(__name__) -def run_validate(app_root: str) -> int: +def run_validate(app_root: str, strict: bool = False) -> int: """Validate ``app.yaml`` and discovered ``agent.yaml`` files. Runs the same structural checks as runtime (``validate_*`` helpers). Prints issues to the log and returns 1 if any warning-level issue is found (suitable for CI). + Findings come in two severities. **Warnings** are structural problems and + fail the run. **Advisories** are heuristic — a likely misconfiguration that + may well be deliberate (e.g. a channel override set for one sibling channel + but not another). They are printed but do not affect the exit code, so + adding a heuristic lint cannot break an existing pipeline; ``strict`` + promotes them to warnings for teams that want them enforced. + Args: app_root: Application root directory containing ``app.yaml``. + strict: Treat advisories as warnings (exit 1 if any are found). Returns: 0 if no issues, 1 otherwise. @@ -60,6 +68,7 @@ def run_validate(app_root: str) -> int: data = resolve_env_placeholders(raw) issues: List[str] = [] + advisories: List[str] = [] for w in validate_app_yaml_descriptor(data): suffix = f" Hint: {w.hint}" if w.hint else "" issues.append(f"app.yaml [{w.path}] {w.message}{suffix}") @@ -79,9 +88,14 @@ def run_validate(app_root: str) -> int: agent_data = resolve_env_placeholders(agent_raw) for agent_issue in validate_agent_yaml(agent_data): suffix = f" Hint: {agent_issue.hint}" if agent_issue.hint else "" - issues.append( - f"{agent_file} [{agent_issue.path}] {agent_issue.message}{suffix}" - ) + line = f"{agent_file} [{agent_issue.path}] {agent_issue.message}{suffix}" + if getattr(agent_issue, "severity", "warning") == "advisory" and not strict: + advisories.append(line) + else: + issues.append(line) + + for line in advisories: + logger.warning("validate advisory: %s", line) if issues: for line in issues: @@ -89,6 +103,15 @@ def run_validate(app_root: str) -> int: logger.error("validate failed: %d issue(s) in %s", len(issues), root) return 1 + if advisories: + logger.info( + "validate OK: %s (%d advisory finding(s) — re-run with --strict to " + "treat them as failures)", + root, + len(advisories), + ) + return 0 + logger.info("validate OK: %s", root) return 0 @@ -107,7 +130,9 @@ def print_usage() -> None: jvagent [run] [--debug] --stress-seed --user-memory-nodes N --interactions-per-user-memory-node M ... After bootstrap, populate the memory graph, then start the server (same DB) jvagent [] status Show application status - jvagent [] validate Check app.yaml and agent.yaml structure (exit 1 if issues; for CI) + jvagent [] validate [--strict] Check app.yaml and agent.yaml structure (exit 1 if issues; for CI) + --strict: also fail on advisory findings (heuristic lints, + e.g. a channel override missing its sibling channel) jvagent [] stress-seed --user-memory-nodes N --interactions-per-user-memory-node M Seed synthetic User + Interaction graph (stress testing) jvagent [] bootstrap [--update] Bootstrap application graph diff --git a/jvagent/core/agent_yaml_validator.py b/jvagent/core/agent_yaml_validator.py index 5b841495..c57ef0f3 100644 --- a/jvagent/core/agent_yaml_validator.py +++ b/jvagent/core/agent_yaml_validator.py @@ -6,6 +6,11 @@ from dataclasses import dataclass from typing import Any, Dict, Iterable, List, Set +from jvagent.core.channel import ( + CHANNEL_PROVIDERS, + COVERAGE_SENSITIVE_OVERRIDE_KEYS, + channel_siblings, +) from jvagent.core.yaml_validation_utils import expect_type as expect_type_generic from jvagent.core.yaml_validation_utils import warn_once as warn_once_generic from jvagent.core.yaml_validation_utils import ( @@ -17,11 +22,19 @@ @dataclass(frozen=True) class AgentYamlWarning: - """Single agent.yaml validation warning.""" + """Single agent.yaml validation finding. + + ``severity`` is ``"warning"`` (structural problems — these fail + ``jvagent validate``) or ``"advisory"`` (a likely-but-not-certain + misconfiguration, printed without affecting the exit code unless + ``--strict``). Advisories exist so a heuristic lint can ship without + breaking the CI of apps whose config is deliberate. + """ path: str message: str hint: str = "" + severity: str = "warning" _SEEN_WARNING_KEYS: Set[str] = set() @@ -46,6 +59,74 @@ def _mk(path: str, message: str, hint: str = "") -> AgentYamlWarning: return AgentYamlWarning(path=path, message=message, hint=hint) +def _mk_advisory(path: str, message: str, hint: str = "") -> AgentYamlWarning: + return AgentYamlWarning(path=path, message=message, hint=hint, severity="advisory") + + +def _sets_override_key(cfg: Any, key: str) -> bool: + """True if a per-channel override block sets *key* at all.""" + return isinstance(cfg, dict) and key in cfg + + +def _check_channel_override_coverage( + warnings: List[AgentYamlWarning], + path: str, + context: Any, + enabled_action_refs: Set[str], +) -> None: + """Advise when a channel knob is set for one sibling channel but not another. + + ``channel_overrides`` is resolved by the EXACT ``visitor.channel`` string, so + a block written for ``whatsapp`` does nothing on a ``whatsapp_call`` (voice) + turn — the action-level value applies instead, silently. Both keys are valid, + so this is not a typo any key-validity check could catch; the only signal is + that one member of a channel family is configured and its reachable sibling + is not. + + Deliberately conservative: it fires only when the sibling channel is actually + reachable (its providing action is enabled on this agent), and only for keys + whose absence changes behavior silently. Anything else would be noise, and a + lint operators learn to ignore is worse than no lint. + """ + if not isinstance(context, dict): + return + overrides = context.get("channel_overrides") + if not isinstance(overrides, dict): + return + + for channel, cfg in overrides.items(): + if not isinstance(cfg, dict): + continue + channel_name = str(channel) + for sibling in sorted(channel_siblings(channel_name)): + provider = CHANNEL_PROVIDERS.get(sibling) + if not provider or provider not in enabled_action_refs: + continue # not reachable here — saying anything would be noise + for key in COVERAGE_SENSITIVE_OVERRIDE_KEYS: + if not _sets_override_key(cfg, key): + continue + if _sets_override_key(overrides.get(sibling), key): + continue + warnings.append( + _mk_advisory( + f"{path}.context.channel_overrides", + ( + f"'{key}' is overridden for channel " + f"'{channel_name}' but not for its sibling " + f"'{sibling}', which is reachable on this agent " + f"(provided by {provider}). Turns on " + f"'{sibling}' will use the action-level " + f"'{key}' instead." + ), + hint=( + f"channel_overrides is matched on the exact channel " + f"string. Add a '{sibling}' block setting '{key}' if " + f"that is not intended." + ), + ) + ) + + def _warn_once(warnings: Iterable[AgentYamlWarning], source: str) -> None: warn_once_generic( warnings=warnings, @@ -103,6 +184,15 @@ def validate_agent_yaml(data: Dict[str, Any]) -> List[AgentYamlWarning]: warnings.append(_mk("actions", f"Expected list, got {type(actions).__name__}")) return warnings + # Which action references this agent enables — needed before the per-entry + # pass, since a channel's reachability depends on a *different* entry than + # the one carrying the override block. + enabled_action_refs: Set[str] = { + str(entry.get("action")) + for entry in actions + if isinstance(entry, dict) and isinstance(entry.get("action"), str) + } + orchestrator_count = 0 for idx, action_entry in enumerate(actions): path = f"actions[{idx}]" @@ -140,6 +230,9 @@ def validate_agent_yaml(data: Dict[str, Any]) -> List[AgentYamlWarning]: _expect_type(warnings, f"{path}.context", action_entry.get("context"), (dict,)) _expect_type(warnings, f"{path}.config", action_entry.get("config"), (dict,)) + _check_channel_override_coverage( + warnings, path, action_entry.get("context"), enabled_action_refs + ) if orchestrator_count > 1: warnings.append( diff --git a/jvagent/core/channel.py b/jvagent/core/channel.py index f31822a3..88558db6 100644 --- a/jvagent/core/channel.py +++ b/jvagent/core/channel.py @@ -1,10 +1,56 @@ -"""Channel normalization utilities. +"""Channel normalization utilities and the channel topology config lints use. The canonical channel for web/default UI is 'default'. When no channel is specified or 'web' is used, it normalizes to 'default'. + +Channels are an **open set**: ``/interact`` accepts ``channel`` as a free-form +query parameter, so any caller can introduce one. The tables below are therefore +advisory metadata for validation, never an allow-list — an unrecognized channel +is legitimate and must never be rejected. """ -from typing import Optional +from typing import Dict, FrozenSet, Optional, Tuple + +# Channels that arrive as separate strings but describe one integration. A knob +# set for one member and not the other is usually an oversight: the operator +# thinks of "WhatsApp" as one thing, but a voice turn arrives on +# ``whatsapp_call`` and a chat turn on ``whatsapp``, and ``channel_overrides`` +# is looked up by the exact string. +CHANNEL_FAMILIES: Tuple[FrozenSet[str], ...] = ( + frozenset({"whatsapp", "whatsapp_call"}), +) + +# Which first-party action reference makes a channel reachable on an agent. Used +# to decide whether a missing sibling override is worth mentioning: an agent with +# no voice action should never be told about ``whatsapp_call``. Third-party +# adapters are absent by design — an unknown provider yields no advisory rather +# than a wrong one. +# Keys are the canonical ``package.name`` from each action's info.yaml — NOT the +# directory name, which differs (``jvagent/action/whatsapp_voice/`` publishes as +# ``jvagent/whatsapp_voice_action``). agent.yaml references the package name. +CHANNEL_PROVIDERS: Dict[str, str] = { + "whatsapp": "jvagent/whatsapp_action", + "whatsapp_call": "jvagent/whatsapp_voice_action", + "email": "jvagent/email_action", + "messenger": "jvagent/facebook_action", +} + +# Override keys whose absence on a channel changes behavior *silently*. Excluded +# deliberately: history_limit, ack knobs, system_prompt_extra — per-channel +# divergence there is normal and intentional, so flagging it would be noise. +COVERAGE_SENSITIVE_OVERRIDE_KEYS: Tuple[str, ...] = ( + "skill_only_tools", + "denied_tools", + "pinned_tools", +) + + +def channel_siblings(channel: str) -> FrozenSet[str]: + """Other channels in *channel*'s family (empty when it has none).""" + for family in CHANNEL_FAMILIES: + if channel in family: + return frozenset(family - {channel}) + return frozenset() def normalize_channel(channel: Optional[str]) -> str: diff --git a/tests/action/orchestrator/test_skill_only_channel_overrides.py b/tests/action/orchestrator/test_skill_only_channel_overrides.py index 925da720..c439660f 100644 --- a/tests/action/orchestrator/test_skill_only_channel_overrides.py +++ b/tests/action/orchestrator/test_skill_only_channel_overrides.py @@ -176,7 +176,9 @@ async def test_channel_resolved_list_drives_the_lean_pool( """Under lean, gated names are excluded from the pre-surface candidate pool. That exclusion must follow the CHANNEL-resolved list, not the action-level one — otherwise a channel that ungates a tool still can't surface it.""" - many = _PAY + [(f"misc__t{i:02d}", f"Miscellaneous capability {i}") for i in range(20)] + many = _PAY + [ + (f"misc__t{i:02d}", f"Miscellaneous capability {i}") for i in range(20) + ] ex = make_orchestrator(actions=[_ToolsAction(many)]) ex.lean_tool_threshold = 5 # force lean on ex.lean_presurface_k = 3 diff --git a/tests/cli/test_validate_advisories.py b/tests/cli/test_validate_advisories.py new file mode 100644 index 00000000..5dcf88e4 --- /dev/null +++ b/tests/cli/test_validate_advisories.py @@ -0,0 +1,80 @@ +"""``jvagent validate`` exit codes for advisory findings. + +An advisory is a heuristic lint. It must be visible but must NOT fail an +existing pipeline — every current `AgentYamlWarning` is CI-fatal, so shipping a +heuristic at warning severity would break the build of any app whose config is +deliberately asymmetric. ``--strict`` is the opt-in for teams that want them +enforced. +""" + +from __future__ import annotations + +from pathlib import Path + +import yaml + +from jvagent.cli.validate import run_validate + +_APP_YAML = { + "app": "probe_app", + "version": "1.0.0", + "author": "test", + "context": {"name": "Probe App", "description": "advisory exit-code probe"}, + "agents": ["probe/bot"], +} + + +def _write_app(root: Path, overrides: dict) -> None: + """An app whose only finding is the sibling-coverage advisory.""" + root.mkdir(parents=True, exist_ok=True) + (root / "app.yaml").write_text(yaml.safe_dump(_APP_YAML), encoding="utf-8") + agent_dir = root / "agents" / "probe" / "bot" + agent_dir.mkdir(parents=True, exist_ok=True) + (agent_dir / "agent.yaml").write_text( + yaml.safe_dump( + { + "agent": "bot", + "version": "0.0.1", + "author": "test", + "jvagent": "0.1.0", + "actions": [ + { + "action": "jvagent/orchestrator", + "context": {"channel_overrides": overrides}, + }, + {"action": "jvagent/whatsapp_voice_action"}, + ], + } + ), + encoding="utf-8", + ) + + +def test_advisory_does_not_fail_validate(tmp_path): + _write_app(tmp_path, {"whatsapp": {"skill_only_tools": []}}) + assert run_validate(str(tmp_path)) == 0 + + +def test_strict_promotes_advisory_to_failure(tmp_path): + _write_app(tmp_path, {"whatsapp": {"skill_only_tools": []}}) + assert run_validate(str(tmp_path), strict=True) == 1 + + +def test_clean_config_passes_under_strict(tmp_path): + """--strict must not invent findings — a covered config still passes.""" + _write_app( + tmp_path, + { + "whatsapp": {"skill_only_tools": []}, + "whatsapp_call": {"skill_only_tools": []}, + }, + ) + assert run_validate(str(tmp_path), strict=True) == 0 + + +def test_advisory_is_logged(tmp_path, caplog): + _write_app(tmp_path, {"whatsapp": {"skill_only_tools": []}}) + with caplog.at_level("WARNING"): + run_validate(str(tmp_path)) + assert "validate advisory" in caplog.text + assert "whatsapp_call" in caplog.text diff --git a/tests/core/test_channel_override_coverage.py b/tests/core/test_channel_override_coverage.py new file mode 100644 index 00000000..7f8cf3ed --- /dev/null +++ b/tests/core/test_channel_override_coverage.py @@ -0,0 +1,253 @@ +"""Sibling-coverage advisory for ``channel_overrides`` (config lint). + +``channel_overrides`` is resolved by the EXACT ``visitor.channel`` string, so a +block written for ``whatsapp`` does nothing on a ``whatsapp_call`` (voice) turn — +the action-level value applies instead, silently. Both keys are valid, so no +key-validity check can catch it; the only available signal is that one member of +a channel family is configured while its reachable sibling is not. + +The lint is advisory: it is a heuristic about intent, and a deliberate +asymmetry is legitimate. It must never fail ``jvagent validate`` by default. +""" + +from __future__ import annotations + +from pathlib import Path +from typing import Any, Dict, List + +from jvagent.core.agent_yaml_validator import ( + AgentYamlWarning, + _reset_warning_cache_for_tests, + validate_agent_yaml, +) + + +def _agent(actions: List[Dict[str, Any]]) -> Dict[str, Any]: + return { + "agent": "test_agent", + "version": "0.0.1", + "author": "test", + "jvagent": "0.1.0", + "actions": actions, + } + + +def _orchestrator(overrides: Dict[str, Any]) -> Dict[str, Any]: + return { + "action": "jvagent/orchestrator", + "context": {"channel_overrides": overrides}, + } + + +def _advisories(data: Dict[str, Any]) -> List[AgentYamlWarning]: + _reset_warning_cache_for_tests() + return [w for w in validate_agent_yaml(data) if w.severity == "advisory"] + + +def _warnings(data: Dict[str, Any]) -> List[AgentYamlWarning]: + _reset_warning_cache_for_tests() + return [w for w in validate_agent_yaml(data) if w.severity == "warning"] + + +def test_missing_sibling_override_is_advised(): + """The reported incident: skill_only_tools set for whatsapp only, with the + voice action enabled, so voice turns silently use the action-level list.""" + data = _agent( + [ + _orchestrator({"whatsapp": {"skill_only_tools": []}}), + {"action": "jvagent/whatsapp_action"}, + {"action": "jvagent/whatsapp_voice_action"}, + ] + ) + found = _advisories(data) + assert len(found) == 1 + message = found[0].message + assert "skill_only_tools" in message + assert "'whatsapp'" in message and "'whatsapp_call'" in message + # The consequence is the actionable part — the omission alone is often fine. + assert "will use the action-level" in message + assert "jvagent/whatsapp_voice_action" in message + assert found[0].path.endswith("channel_overrides") + + +def test_advisory_does_not_fail_validation(): + """Advisories must not surface as warnings — they must not break CI.""" + data = _agent( + [ + _orchestrator({"whatsapp": {"denied_tools": ["pay__*"]}}), + {"action": "jvagent/whatsapp_voice_action"}, + ] + ) + assert _advisories(data) + assert _warnings(data) == [] + + +def test_no_advisory_when_the_sibling_is_unreachable(): + """No voice action installed → voice turns cannot occur → say nothing.""" + data = _agent( + [ + _orchestrator({"whatsapp": {"skill_only_tools": []}}), + {"action": "jvagent/whatsapp_action"}, + ] + ) + assert _advisories(data) == [] + + +def test_no_advisory_when_both_siblings_set_the_key(): + data = _agent( + [ + _orchestrator( + { + "whatsapp": {"skill_only_tools": []}, + "whatsapp_call": {"skill_only_tools": ["pay__*"]}, + } + ), + {"action": "jvagent/whatsapp_voice_action"}, + ] + ) + assert _advisories(data) == [] + + +def test_sibling_block_exists_but_omits_the_key(): + """A sibling block that sets other knobs still misses THIS one.""" + data = _agent( + [ + _orchestrator( + { + "whatsapp": {"skill_only_tools": []}, + "whatsapp_call": {"history_limit": 4}, + } + ), + {"action": "jvagent/whatsapp_voice_action"}, + ] + ) + found = _advisories(data) + assert len(found) == 1 + assert "skill_only_tools" in found[0].message + + +def test_non_coverage_sensitive_keys_are_ignored(): + """Per-channel divergence in history_limit / acks / prompt extra is normal.""" + data = _agent( + [ + _orchestrator( + { + "whatsapp": { + "history_limit": 4, + "system_prompt_extra": "be brief", + "ack_statements": ["one moment"], + } + } + ), + {"action": "jvagent/whatsapp_voice_action"}, + ] + ) + assert _advisories(data) == [] + + +def test_advisory_is_symmetric(): + """Gating voice but not chat is as likely a mistake as the reverse.""" + data = _agent( + [ + _orchestrator({"whatsapp_call": {"pinned_tools": ["wa__send_flow"]}}), + {"action": "jvagent/whatsapp_action"}, + {"action": "jvagent/whatsapp_voice_action"}, + ] + ) + found = _advisories(data) + assert len(found) == 1 + assert "'whatsapp_call'" in found[0].message + assert "jvagent/whatsapp" in found[0].message + + +def test_one_advisory_per_missing_key(): + data = _agent( + [ + _orchestrator( + {"whatsapp": {"skill_only_tools": [], "denied_tools": ["x__*"]}} + ), + {"action": "jvagent/whatsapp_voice_action"}, + ] + ) + found = _advisories(data) + assert len(found) == 2 + flagged = { + key + for key in ("skill_only_tools", "denied_tools", "pinned_tools") + if any(f"'{key}'" in w.message for w in found) + } + assert flagged == {"skill_only_tools", "denied_tools"} + + +def test_channel_with_no_family_is_ignored(): + """``email`` has no sibling — nothing to compare against.""" + data = _agent( + [ + _orchestrator({"email": {"skill_only_tools": []}}), + {"action": "jvagent/email_action"}, + {"action": "jvagent/whatsapp_voice_action"}, + ] + ) + assert _advisories(data) == [] + + +def test_no_channel_overrides_produces_no_advisories(): + data = _agent([{"action": "jvagent/orchestrator", "context": {}}]) + assert _advisories(data) == [] + + +def test_malformed_override_block_is_ignored_not_crashed(): + """A non-mapping block is someone else's warning to raise, not a crash here.""" + data = _agent( + [ + _orchestrator({"whatsapp": ["not", "a", "mapping"]}), + {"action": "jvagent/whatsapp_voice_action"}, + ] + ) + assert _advisories(data) == [] + + +def test_channel_providers_match_real_action_package_names(): + """Every CHANNEL_PROVIDERS ref must be a real action's ``package.name``. + + This is the test that matters most here. The provider refs are the lint's + reachability gate, so a wrong one makes the whole check silently never fire + — and every behavioral test above still passes, because they use the same + wrong constant as their fixture. The first draft of this map used the + package DIRECTORY names (`jvagent/whatsapp_voice`) rather than the published + package names (`jvagent/whatsapp_voice_action`), which differ. Only reading + the real info.yaml catches that. + """ + import yaml + + from jvagent.core.channel import CHANNEL_PROVIDERS + + root = Path(__file__).resolve().parents[2] / "jvagent" / "action" + published = set() + for info in root.rglob("info.yaml"): + try: + data = yaml.safe_load(info.read_text(encoding="utf-8")) or {} + except Exception: # pragma: no cover - unreadable package metadata + continue + name = (data.get("package") or {}).get("name") + if isinstance(name, str): + published.add(name) + + assert published, "no action packages discovered — test harness is broken" + unknown = { + channel: ref + for channel, ref in CHANNEL_PROVIDERS.items() + if ref not in published + } + assert not unknown, ( + f"CHANNEL_PROVIDERS references non-existent action packages: {unknown}. " + "Use the 'package.name' from the action's info.yaml, not its directory." + ) + + +def test_existing_warnings_keep_default_severity(): + """The new field must not reclassify any pre-existing structural warning.""" + _reset_warning_cache_for_tests() + found = validate_agent_yaml(_agent([{"action": "no_namespace"}])) + assert found + assert all(w.severity == "warning" for w in found)