fix: let bare Tools compete directly in delegate selection (not just as a last resort) - #195
fix: let bare Tools compete directly in delegate selection (not just as a last resort)#195imaustink wants to merge 6 commits into
Conversation
"Is airvinyl running okay" matched claude-code-swe-agent -- the only
Agent in the catalog, description broad enough ("runs bash...") to
loosely overlap via embedding similarity -- instead of the ssh tool,
and hung on that agent's Claude identity-link gate (a known class of
pre-existing bug in this repo: fix/claude-auth-submit-hang et al.).
Nothing about that path is specific to ssh; any bare Tool with no
wrapping Skill is exposed to the same mis-routing risk once a broad
Agent exists in the catalog.
Adds ssh-skill (mirroring cluster-debug-skill's shape: toolRefs: [ssh],
no allowedRoles of its own per ADR 0011), so selectDelegate's combined
skill+agent choice sees a strong, specific match for SSH-shaped
requests and picks it over the vague agent match. Its markdown also
gives authored guidance for turning an open-ended "is it healthy?"
into a concrete sequence of read-only diagnostic calls (uptime, df -h,
free -m, then a named service if implied) -- separately addressing
why a vague request was declining at the ActionPlanner stage even when
the ssh tool WAS the right fallback candidate.
Verified with a real kind cluster + kubectl apply --dry-run=server.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Fg8b9pPWm91nLbDnB6ECJh
…tion Fixes the actual root cause behind ssh-skill being needed at all: a bare Tool with no wrapping Skill was only ever considered as a last resort (noMatchFallback's selectFallbackTool), invoked only once skillCandidates AND agentCandidates both came up empty. Any Agent whose description loosely overlapped a request via embedding similarity alone -- not because it was actually the better fit, but because a Tool never got the chance to compete at all -- would win by existing as a candidate. That's what let claude-code-swe-agent (the only Agent in the catalog) absorb "SSH into X" requests instead of the ssh Tool, hanging on its identity-link gate. Adds a `retrieveTools` graph node (embedding query + ToolFitChecker, reusing selectFallbackTool's own two-stage relevance gate) alongside retrieveSkills/retrieveAgents, and extends DelegateSelector to a three-way choice among skills/agents/tools in one combined decision (docs/adr/0036). Guarded on deps.delegateSelector being configured, so non-NATS deployments pay no extra cost and keep the exact old skill-only + selectFallbackTool behavior. ssh-skill (this branch's other commit) stays -- it still adds value a bare Tool alone can't: authored guidance for turning an open-ended "is it healthy?" into a concrete sequence of diagnostic calls. This fixes the starvation itself so no *future* unwrapped Tool needs a Skill just to be reachable. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Fg8b9pPWm91nLbDnB6ECJh
imaustink
changed the title
|
🤖 Starting work on this now. |
|
🤖 Watch live or take over the session here: https://claude.ai/code/session_01Bn6S7LNrrgd7nfGXtVrddZ |
|
Something went wrong processing this: /invoke/96006fd2-e3e7-4e21-a8af-75f0de005d07 poll failed: 404 |
![k5s-bot[bot]](https://avatars.githubusercontent.com/in/1052865?s=60&v=4){kind=small}
There was a problem hiding this comment.
Solid, well-scoped fix — retrieveTools correctly reuses the existing embedding query + ToolFitChecker gate, the three-way selector change is clean, and the tests cover the important routing cases. Two issues before merge: one user-facing correctness bug on the new tool branch, and one drift risk in the un-templated skill markdown. (No CI checks are configured on this branch, so nothing was running to gate this review.)
|
We probably need to implement the same fix for the Temporal side, right? |
|
🤖 Starting work on this now. |
|
🤖 Watch live or take over the session here: https://claude.ai/code/session_01RDDEgbKjLxXbavFPsJFyWk |
… values
- graph.ts: a tool deliberately picked by delegateSelector in the three-way
comparison is a first-class match, not a fallback. Drop the erroneous
`wasFallback: true` that appended the self-improvement ("nothing matched")
footer to every request this branch successfully routed. Genuine no-matches
still flow through noMatchFallback, which sets the flag correctly.
- graph.test.ts: assert wasFallback stays false when a tool is chosen.
- skill-ssh.yaml: derive the skill markdown from .Values.sshTool (the same
source the ssh Tool renders from) instead of hardcoding author-specific
facts. Gate the write-capability claim on allowedCommands=="*" (read-only
guidance otherwise) and render the target list from sshConfig/allowedHosts,
so the skill can't drift from the Tool on non-author deployments.
# Conflicts: # docs/adr/README.md
Ports ADR 0037 to the Temporal execution engine, which had the identical asymmetry the langgraph engine did: runAgentTurn retrieved only skills and agents, SelectDelegate chose skill-vs-agent, and a bare Tool was reachable only via noMatchFallback's selectFallbackTool — so a broad Agent pre-empted a better-fitting Tool there too (the maintainer flagged this on #195). - activities/delegate.go: SelectDelegate gains a Tools input and a "tool" DelegateChoice, with a skill > tool > agent preference order in its prompt; a chosen tool id is validated against the offered set (hallucinations fail to no-match, like SelectSkill). - workflows/agentloop.go: fit-check catalog tools (reusing fitCandidates + retrieveCatalogTools) between agent retrieval and selection, and offer the survivors to the combined selector when agents OR tools exist. A "tool" choice runs first-class via runSelectedTool (meta.Path "tool", NO self-improvement footer); a planner decline or empty choice falls through to noMatchFallback's safety net. - workflows/fallback.go: extract shared planToolCall (from selectFallbackTool's tail) and runToolCall (footer-parameterised core of runFallbackTool); runSelectedTool is the footer-less first-class variant. - Tests: activity-level 3-way SelectDelegate validation, plus workflow tests for a tool winning over an agent (first-class, agent never launched, no footer) and a fit-gate-rejected tool being excluded from the selector. Docs: extend ADR 0037 with a "Temporal engine" section describing the port.
|
@imaustink yes — you're right, the Temporal engine had the identical asymmetry, and I've ported the same fix in
|
Picked this PR back up — summary of changesReview feedback addressed
Branch sync Merged Verification
Deliberately not done
|
2 participants
Summary
Follow-up to #192. After that PR deployed, "Can you SSH into airvinyl and see if it's running all good?" in Open WebUI got a generic LLM fallback with no tool use, and a follow-up with a specific command ("SSH into airvinyl and run uptime") got stuck showing "found 1 agent candidate" indefinitely.
Root cause, confirmed against the live cluster:
claude-code-swe-agentis the onlyAgentin the whole catalog, and its broad description ("Performs software-engineering work on GitHub end-to-end. Runs the Claude Code CLI headless — which has bash, file-read/write, grep, and glob tools...") loosely matched via embedding similarity.selectDelegateonly ever considered a bareTool(likessh, with no wrappingSkill) as a last resort — reachable only onceskillCandidatesANDagentCandidatesboth came up empty. Since an Agent candidate always existed, thesshTool never got a chance to compete at all, and dispatching to the agent hung on its Claude identity-link gate (a known, pre-existing bug class —fix/claude-auth-submit-hanget al. — unrelated to #192's code).Fix
1. The core root cause (
docs/adr/0037-tools-compete-directly-in-delegate-selection.md) — a bare Tool was starved out categorically, not because it lost a comparison but because it was never in the comparison. Any future unwrapped Tool would hit the identical problem the moment a broad Agent (or Skill) exists in the catalog. Fixed in both execution engines:langgraph engine (
apps/agent-orchestrator)retrieveToolsnode (agent/graph.ts) inserted betweenretrieveAgentsandselectDelegate: runs the same embedding query +ToolFitCheckertwo-stage relevance gateselectFallbackToolalready used. Guarded ondeps.delegateSelectorbeing configured, so non-NATS deployments pay no extra cost and keep the exact old behavior.DelegateSelector.select(agent/delegate-selector.ts) takes a thirdtoolsparameter and makes one combined three-way choice (skill/agent/tool), with an explicit preference order: skill (authored guidance) > bare tool (single well-defined action) > agent (open-ended/multi-step work).selectDelegate's tool branch reuses a new shared helper (planFallbackToolCall, extracted fromselectFallbackTool's tail) to construct the actual call, and treats it as a first-class selection (no self-improvement footer), falling back tonoMatchFallback/selectFallbackToolonly if the planner declines or the combined choice comes back empty.Temporal engine (
engines/temporal) — added in this update, in response to @imaustink's review question. The Go engine had the identical asymmetry:SelectDelegate(activities/delegate.go) gains aToolsinput and a"tool"DelegateChoicewith the same skill > tool > agent preference order, validating the chosen id against the offered set.runAgentTurn(workflows/agentloop.go) fit-checks catalog tools (reusingfitCandidates+retrieveCatalogTools) and offers survivors to the combined selector. A"tool"choice runs first-class viarunSelectedTool(meta.Pathtool, no self-improvement footer); a decline falls through tonoMatchFallback.fallback.gorefactored: sharedplanToolCall+ footer-parameterisedrunToolCall.2.
ssh-skill(charts/community-components/templates/skill-ssh.yaml) — kept alongside the core fix (it still adds authored multi-step diagnostic guidance a bare Tool alone can't). Its markdown is now derived from.Values.sshToolrather than hardcoded, so it can't drift from thesshTool on non-author deployments: the write-capability section is gated onallowedCommands == "*"(read-only guidance otherwise), and the target list is rendered fromsshConfig/allowedHosts.Review feedback addressed
wasFallback: trueon the tool branch (graph.ts) — removed; a deliberately-selected tool is a first-class match and must not append the "nothing matched" footer. Regression assertion added.skill-ssh.yaml) — now derived from.Values.sshTool(see above).Test plan
npm run typecheck/build/test --workspace=agent-orchestrator— 595/595 passing (incl. thewasFallbackregression assertion)gofmt -lclean,go build ./...,go vet ./...,go test ./...— all pass, incl. 3 new tests (3-waySelectDelegatevalidation; a tool winning over an agent first-class with the agent never launched and no footer; a fit-gate-rejected tool excluded from the selector)helm lint+helm templateagainstvalues.yaml,values-production.yaml,values-ci-all.yaml; all 33 community-components templates render and the output parses; renderedssh-skillmarkdown matches each deployment's configured commands + hosts🤖 Generated with Claude Code
https://claude.ai/code/session_01RDDEgbKjLxXbavFPsJFyWk